nodes/*funcs.c inconsistencies

I observed these inconsistencies in node support functions:

- _copyReassignOwnedStmt() uses COPY_SCALAR_FIELD() on the string field
"newrole", and _equalReassignOwnedStmt() uses COMPARE_NODE_FIELD().

- _outCreateForeignTableStmt() calls _outCreateStmt() directly. This produces
the label "CREATEFOREIGNTABLESTMTCREATESTMT". The attached patch splits
things out the way we normally do in outfuncs.c. There's no readfuncs.c
support, so this is strictly cosmetic.

- _outColumnDef() uses WRITE_INT_FIELD for the "storage" field, a char.
Again, no readfuncs.c support to create a compatibility problem.

- _copyRenameStmt() and _equalRenameStmt() ignore the "relationType" field,
but I can't see a good reason to do so. PostgreSQL 9.1 added this field,
but only recent master (after commit 38b9693f of 3 April 2012) references
the field beyond setting it in the parser.

- _copyViewStmt() and _equalViewStmt() ignore the "options" field, and
_equalColumnDef() ignores "fdwoptions". These are new in PostgreSQL 9.2,
and I see no good reason to ignore them.

I'd suggest backpatching the ReassignOwnedStmt() bits; the wrong code could
produce crashes. The rest are for master only.

Thanks,
nm

On Mon, Apr 16, 2012 at 6:25 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> I'd suggest backpatching the ReassignOwnedStmt() bits; the wrong code could
> produce crashes.  The rest are for master only.

Done, in the manner you suggest.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Excerpts from Robert Haas's message of mié abr 18 11:47:37 -0300 2012:
> On Mon, Apr 16, 2012 at 6:25 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > I'd suggest backpatching the ReassignOwnedStmt() bits; the wrong code could
> > produce crashes.  The rest are for master only.
>
> Done, in the manner you suggest.

Pah. I was just done with it and was about to push it ...

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote:
> I observed these inconsistencies in node support functions:

A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic
INT/UINT or field order corrections. The non-cosmetic changes involve
CustomPath, CustomScan, and CreatePolicyStmt. Feature committers, if the
existing treatments (ignore custom_plans/custom_paths fields; copy/compare
"cmd" string pointer as a scalar) were deliberate, please let me know.

Thanks,
nm

[1] The _equalCreateEventTrigStmt() cosmetic change is relevant as far back as
9.3, but I won't back-patch it further than the others (9.5).

Noah Misch <noah(at)leadboat(dot)com> writes:
> On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote:
>> I observed these inconsistencies in node support functions:

> A fresh audit found the attached problems new in 9.5[1].

Many thanks for doing that; I'd had the same checking on my personal to-do
list, but now will not need to.

regards, tom lane

On Sun, Aug 2, 2015 at 1:28 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic
> INT/UINT or field order corrections.

I was responsible for a couple of the cosmetic ones. Sorry about that.

It occurs to me that we could do a little more to prevent this
automatically. Couldn't we adopt
AssertVariableIsOfType()/AssertVariableIsOfTypeMacro() to macros like
READ_UINT_FIELD()?

I'm surprised that this stuff was only ever used for logical decoding
infrastructure so far.
--
Peter Geoghegan

On Sun, Aug 2, 2015 at 3:07 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> I'm surprised that this stuff was only ever used for logical decoding
> infrastructure so far.

On second thought, having tried it, one reason is that that breaks
things that are considered legitimate for historical reasons. For
example, AttrNumber is often used with READ_INT_FIELD(), which is an
int16. Whether or not it's worth fixing that by introducing a
READ_ATTRNUM_FIELD() (and so on) is not obvious to me.

--
Peter Geoghegan

Noah,

* Noah Misch (noah(at)leadboat(dot)com) wrote:
> On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote:
> > I observed these inconsistencies in node support functions:
>
> A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic
> INT/UINT or field order corrections. The non-cosmetic changes involve
> CustomPath, CustomScan, and CreatePolicyStmt. Feature committers, if the
> existing treatments (ignore custom_plans/custom_paths fields; copy/compare
> "cmd" string pointer as a scalar) were deliberate, please let me know.

Thanks for the review. The change you have is correct for
CreatePolicyStmt, at least. I imagine I confused it with polcmd, which
is actually just a char.

Barring objections, I'll change it to cmd_name after your commit, to
reduce the chances of future confusion.

Thanks again!

Stephen

> On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote:
> > I observed these inconsistencies in node support functions:
>
> A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic
> INT/UINT or field order corrections. The non-cosmetic changes involve
> CustomPath, CustomScan, and CreatePolicyStmt. Feature committers, if the
> existing treatments (ignore custom_plans/custom_paths fields; copy/compare
> "cmd" string pointer as a scalar) were deliberate, please let me know.
>
Thanks for your works.

I also noticed one other inconsistent point; _outMergeJoin() dumps
mergeNullsFirst[] array but it does not use booltostr() macro.

Best regards,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On Mon, Aug 03, 2015 at 01:32:10AM +0000, Kouhei Kaigai wrote:
> > On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote:
> > > I observed these inconsistencies in node support functions:
> >
> > A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic
> > INT/UINT or field order corrections. The non-cosmetic changes involve
> > CustomPath, CustomScan, and CreatePolicyStmt. Feature committers, if the
> > existing treatments (ignore custom_plans/custom_paths fields; copy/compare
> > "cmd" string pointer as a scalar) were deliberate, please let me know.
> >
> Thanks for your works.
>
> I also noticed one other inconsistent point; _outMergeJoin() dumps
> mergeNullsFirst[] array but it does not use booltostr() macro.

Good catch. All supported branches work that way, and it's not wrong. I
recommend keeping it unchanged.

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> Noah,
>> A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic
>> INT/UINT or field order corrections. The non-cosmetic changes involve
>> CustomPath, CustomScan, and CreatePolicyStmt. Feature committers, if the
>> existing treatments (ignore custom_plans/custom_paths fields; copy/compare
>> "cmd" string pointer as a scalar) were deliberate, please let me know.

> Thanks for the review. The change you have is correct for
> CreatePolicyStmt, at least. I imagine I confused it with polcmd, which
> is actually just a char.

> Barring objections, I'll change it to cmd_name after your commit, to
> reduce the chances of future confusion.

Both of you please keep in mind that these "cosmetic" changes are
initdb-forcing, at least if they affect node types that can appear
in stored rules.

That being the case, it would probably be a good idea to get them done
before alpha2, as there may not be a good opportunity afterwards.

regards, tom lane

On Sun, Aug 02, 2015 at 11:31:16PM -0400, Tom Lane wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > Noah,
> >> A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic
> >> INT/UINT or field order corrections. The non-cosmetic changes involve
> >> CustomPath, CustomScan, and CreatePolicyStmt. Feature committers, if the
> >> existing treatments (ignore custom_plans/custom_paths fields; copy/compare
> >> "cmd" string pointer as a scalar) were deliberate, please let me know.
>
> > Thanks for the review. The change you have is correct for
> > CreatePolicyStmt, at least. I imagine I confused it with polcmd, which
> > is actually just a char.
>
> > Barring objections, I'll change it to cmd_name after your commit, to
> > reduce the chances of future confusion.

The existing identifier seems fine, but won't I mind that change, either.

> Both of you please keep in mind that these "cosmetic" changes are
> initdb-forcing, at least if they affect node types that can appear
> in stored rules.

Right; Stephen's does not force initdb, but some of what I posted does so.

> That being the case, it would probably be a good idea to get them done
> before alpha2, as there may not be a good opportunity afterwards.

Freedom to bump catversion after alpha2 will be barely-distinguishable from
freedom to do so now. I have planned to leave my usual comment period of a
few days, though skipping that would be rather innocuous in this case.

* Noah Misch (noah(at)leadboat(dot)com) wrote:
> On Sun, Aug 02, 2015 at 11:31:16PM -0400, Tom Lane wrote:
> > That being the case, it would probably be a good idea to get them done
> > before alpha2, as there may not be a good opportunity afterwards.
>
> Freedom to bump catversion after alpha2 will be barely-distinguishable from
> freedom to do so now. I have planned to leave my usual comment period of a
> few days, though skipping that would be rather innocuous in this case.

This is clearly necessary, of course, and I wouldn't be surprised if we
have another necessary bump post-alpha2, but at the same time, it'd
certainly be nice if we are able to put out alpha2 and then a beta1 in
the future without needing to do a bump.

In other words, +1 from me for going ahead and committing this for
alpha2, if possible.

Thanks!

Stephen

Peter Geoghegan wrote:
> On Sun, Aug 2, 2015 at 3:07 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> > I'm surprised that this stuff was only ever used for logical decoding
> > infrastructure so far.
>
> On second thought, having tried it, one reason is that that breaks
> things that are considered legitimate for historical reasons. For
> example, AttrNumber is often used with READ_INT_FIELD(), which is an
> int16. Whether or not it's worth fixing that by introducing a
> READ_ATTRNUM_FIELD() (and so on) is not obvious to me.

If it allows us to introduce additional checking for new code, I'm all
for it.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/03/2015 09:55 AM, Stephen Frost wrote:
> * Noah Misch (noah(at)leadboat(dot)com) wrote:
>> On Sun, Aug 02, 2015 at 11:31:16PM -0400, Tom Lane wrote:
>>> That being the case, it would probably be a good idea to get
>>> them done before alpha2, as there may not be a good opportunity
>>> afterwards.
>>
>> Freedom to bump catversion after alpha2 will be
>> barely-distinguishable from freedom to do so now. I have planned
>> to leave my usual comment period of a few days, though skipping
>> that would be rather innocuous in this case.
>
> This is clearly necessary, of course, and I wouldn't be surprised
> if we have another necessary bump post-alpha2, but at the same
> time, it'd certainly be nice if we are able to put out alpha2 and
> then a beta1 in the future without needing to do a bump.
>
> In other words, +1 from me for going ahead and committing this for
> alpha2, if possible.

+1

- --
Joe Conway
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVv52QAAoJEDfy90M199hl/skP+wXfyo8axMipFwyV9fqesFHF
xRc8j2QHUOVTyqAA9TdksSL1+t8h7a6PmEL6ucIUyGIfcBZaY5kw9/IOQxac8i0W
ZxAElSzFSL8xpimSehZCKLHTF4fTlypSA1srVOb2py6vjOQK+7PZAnlP42TXEgBT
gk6HnAFQBJ6cx8KckYHdqQFPtTKLyglSFLeOCBfbwymOBZgjEopdzTDsE17Z07m8
ZQq08jiSPJKGmwiLI7SCMlA5DMI6B9OV2/h8GXfxP833oR8Z8wQrvzQkM0GnJU5w
Hirk8ECEJndBuLyyVl0suzabNUNdzVWvXQHMIP2+0aW/secAiUbkVdF7o0B5l8qI
GZaHFCZ37BdKgppjApCCLmA/1Th2hHKEwGsIWGiBMQhWfKeRzXxuUZYqSF+YZDRN
ju5ZhJOlmKiLv0AyuMPw9T25X8fTzBwAppjL7r6HM610rNOM0izYz8+7K0ckq74/
jfbt+FuKxGwp8Phz7mm7APOsDsrhNkQgmn1WpnqgJBQwXTRZGzn2ne9npvmUAo3C
4Hiaib2fTIq2Q39scT51b5enjtkPa9S2o4MF+32JP6g5UmLMe3GMpa5V2itvCuN3
ue8AzgNoeyd3oOlqPh9XJi/Kk68it4UacXRkiZAJ9cLTEMixVUjuaVkCyFxsmKxb
vvNa2Of440JgSc2Hw21p
=vdZZ
-----END PGP SIGNATURE-----

Peter Geoghegan wrote:

> Couldn't we adopt
> AssertVariableIsOfType()/AssertVariableIsOfTypeMacro() to macros like
> READ_UINT_FIELD()?
>
> I'm surprised that this stuff was only ever used for logical decoding
> infrastructure so far.

The reason it's only used there is that Andres is the one who introduced
those macros precisely for that code. We've not yet had time to adjust
the rest of the code to have more sanity checks.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Aug 03, 2015 at 09:57:52AM -0700, Joe Conway wrote:
> On 08/03/2015 09:55 AM, Stephen Frost wrote:
> > * Noah Misch (noah(at)leadboat(dot)com) wrote:
> >> On Sun, Aug 02, 2015 at 11:31:16PM -0400, Tom Lane wrote:
> >>> That being the case, it would probably be a good idea to get
> >>> them done before alpha2, as there may not be a good opportunity
> >>> afterwards.
> >>
> >> Freedom to bump catversion after alpha2 will be
> >> barely-distinguishable from freedom to do so now. I have planned
> >> to leave my usual comment period of a few days, though skipping
> >> that would be rather innocuous in this case.
> >
> > This is clearly necessary, of course, and I wouldn't be surprised
> > if we have another necessary bump post-alpha2, but at the same
> > time, it'd certainly be nice if we are able to put out alpha2 and
> > then a beta1 in the future without needing to do a bump.
> >
> > In other words, +1 from me for going ahead and committing this for
> > alpha2, if possible.
>
> +1

Rather than commit on an emergency basis in the few hours between these +1's
and the wrap, I kept to my original schedule. FYI, if it hadn't required
emergency procedures (cancelling the day's plans so I could get to a notebook
computer), I would have committed early based on the three +1s.

Noah,

* Noah Misch (noah(at)leadboat(dot)com) wrote:
> Rather than commit on an emergency basis in the few hours between these +1's
> and the wrap, I kept to my original schedule. FYI, if it hadn't required
> emergency procedures (cancelling the day's plans so I could get to a notebook
> computer), I would have committed early based on the three +1s.

No worries, fully agreed that this was not an emergency in any way.

Thanks!

Stephen

On Mon, Aug 3, 2015 at 10:00 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
>> Couldn't we adopt
>> AssertVariableIsOfType()/AssertVariableIsOfTypeMacro() to macros like
>> READ_UINT_FIELD()?
>>
>> I'm surprised that this stuff was only ever used for logical decoding
>> infrastructure so far.
>
> The reason it's only used there is that Andres is the one who introduced
> those macros precisely for that code. We've not yet had time to adjust
> the rest of the code to have more sanity checks.

Any chance of picking this up, Andres?

--
Peter Geoghegan