Hi Alvaro,
Thanks for looking at this.
On Sat, Jul 27, 2019 at 8:38 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Thanks for the patch!
>
> I'm not completely sold on back-patching this. Should we consider
> changing it in 12beta and up only, or should we just backpatch it all
> the way back to 9.4? It's going to raise errors in cases that
> previously worked.
Applying the fix to only 12beta and up is perhaps fine. AFAICT,
there's no clear design reason for why the attribute storage property
must be the same in all child tables, so most users wouldn't even be
aware that we ensure that in some cases. OTOH, getting an error now
to ensure the invariant more strictly might be more annoying than
helpful.
> On the patch itself: I think ERRCODE_DATATYPE_MISMATCH is not the
> correct one to use for this; maybe ERRCODE_INVALID_OBJECT_DEFINITION or,
> more likely, ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE.
OK, I prefer the latter.
> "FOO versus BAR" does not sound proper style. Maybe "Child table has
> FOO, parent table expects BAR." Or maybe put it all in errmsg,
> errmsg("child table \"%s\" has storage option \"%s\" for column \"%s\" mismatching \"%s\" on parent",
I too found the "FOO versus BAR" style a bit odd, so changed to the
error message you suggest. There are other instances of this style
though:
$ git grep "%s versus %s"
src/backend/commands/tablecmds.c:
errdetail("%s versus %s",
src/backend/commands/tablecmds.c:
errdetail("%s versus %s",
src/backend/commands/tablecmds.c:
errdetail("%s versus %s",
src/backend/commands/tablecmds.c:
errdetail("%s versus %s",
src/backend/parser/parse_coerce.c:
errdetail("%s versus %s",
src/backend/parser/parse_coerce.c:
errdetail("%s versus %s",
src/backend/parser/parse_coerce.c:
errdetail("%s versus %s",
src/backend/parser/parse_coerce.c: errdetail("%s versus %s",
src/backend/parser/parse_coerce.c: errdetail("%s versus %s",
src/backend/parser/parse_param.c: errdetail("%s versus %s",
Should we leave them alone?
Attached updated patch.
Thanks,
Amit