Horton

Thread

Commits

  1. Don't use custom OID symbols in pg_type.dat, either.

  2. Don't use custom OID symbols in pg_proc.dat.

  1. duplicate function oid symbols

    John Naylor <john.naylor@enterprisedb.com> — 2020-10-27T12:43:28Z

    Hi,

I noticed that the table AM abstraction introduced the symbol
HEAP_TABLE_AM_HANDLER_OID, although we already have a convention for
defining symbols automatically for builtin functions, which in this case is
(currently unused) F_HEAP_TABLEAM_HANDLER.

It seems a wart to have two symbols for the same function (seemingly
accidentally). I suppose it's unacceptable to remove the non-standard
symbol since it's been referred to in code for a while now. We could remove
the unused (in core anyway) standard one by arranging fmgroids.h to use
explicit symbols from pg_proc.dat where they exist, as well as prevent such
symbols from being emitted into pg_proc_d.h. But then again there is
no guarantee the standard symbol is not being used elsewhere. Thoughts?

-- 
John Naylor
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  2. Re: duplicate function oid symbols

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-10-27T13:51:16Z

    John Naylor <john.naylor@enterprisedb.com> writes:
> I noticed that the table AM abstraction introduced the symbol
> HEAP_TABLE_AM_HANDLER_OID, although we already have a convention for
> defining symbols automatically for builtin functions, which in this case is
> (currently unused) F_HEAP_TABLEAM_HANDLER.

Yeah, that seems wrong.  I'd just remove HEAP_TABLE_AM_HANDLER_OID.
As long as we're not back-patching the change, it seems like a very
minor thing to fix, if anyone outside core is referencing the old name.

			regards, tom lane

  3. Re: duplicate function oid symbols

    John Naylor <john.naylor@enterprisedb.com> — 2020-10-27T21:40:09Z

    On Tue, Oct 27, 2020 at 9:51 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> John Naylor <john.naylor@enterprisedb.com> writes:
> > I noticed that the table AM abstraction introduced the symbol
> > HEAP_TABLE_AM_HANDLER_OID, although we already have a convention for
> > defining symbols automatically for builtin functions, which in this case
> is
> > (currently unused) F_HEAP_TABLEAM_HANDLER.
>
> Yeah, that seems wrong.  I'd just remove HEAP_TABLE_AM_HANDLER_OID.
> As long as we're not back-patching the change, it seems like a very
> minor thing to fix, if anyone outside core is referencing the old name.
>

Ok, here is a patch to fix that, and also throw an error if pg_proc.dat has
an explicitly defined symbol.

-- 
John Naylor
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  4. Re: duplicate function oid symbols

    Michael Paquier <michael@paquier.xyz> — 2020-10-28T05:26:27Z

    On Tue, Oct 27, 2020 at 05:40:09PM -0400, John Naylor wrote:
> Ok, here is a patch to fix that, and also throw an error if pg_proc.dat has
> an explicitly defined symbol.

I am not seeing any uses of HEAP_TABLE_AM_HANDLER_OID in the wild, so
+1 for just removing it, and not back-patch.
--
Michael

  5. Re: duplicate function oid symbols

    John Naylor <john.naylor@enterprisedb.com> — 2020-10-28T15:44:26Z

    I wrote:


> Ok, here is a patch to fix that, and also throw an error if pg_proc.dat
> has an explicitly defined symbol.
>

It occurred to me I neglected to explain the error with a comment, which
I've added in v2.

-- 
John Naylor
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  6. Re: duplicate function oid symbols

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-10-28T16:25:41Z

    John Naylor <john.naylor@enterprisedb.com> writes:
>> Ok, here is a patch to fix that, and also throw an error if pg_proc.dat
>> has an explicitly defined symbol.

> It occurred to me I neglected to explain the error with a comment, which
> I've added in v2.

Pushed with a bit of tweaking of the error message.

I wondered about introducing a similar prohibition for pg_type.
The only existing oid_symbol in pg_type that I think has enough
grandfather status to be tough to change is CASHOID for "money".
But we could imagine special-casing that with a handmade macro

#define CASHOID MONEYOID

and then getting rid of the oid_symbol entries.  (Or perhaps we
could just up and nuke CASHOID too?  It's somewhat dubious that
any outside code is really using that macro.)

May not be worth the trouble, but if we're anal enough to do this
patch maybe we should do that too.

			regards, tom lane

  7. Re: duplicate function oid symbols

    John Naylor <john.naylor@enterprisedb.com> — 2020-10-28T18:08:28Z

    On Wed, Oct 28, 2020 at 12:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I wondered about introducing a similar prohibition for pg_type.
>

That might be worth doing, since some of the grandfathered macros are
clustered together, which could lead to more cases creeping in as people
match new types to examples nearby.


> The only existing oid_symbol in pg_type that I think has enough
> grandfather status to be tough to change is CASHOID for "money".
> But we could imagine special-casing that with a handmade macro
>
> #define CASHOID MONEYOID
>
> and then getting rid of the oid_symbol entries.  (Or perhaps we
> could just up and nuke CASHOID too?  It's somewhat dubious that
> any outside code is really using that macro.)
>

Yeah, grepping shows that some of those aren't even used in core code. On
the other hand, the difference from the heap_am_handler case is the
standard macros don't already exist for these pg_type entries. The handmade
macro idea could be used for all eight just as easily as for one.

-- 
John Naylor
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  8. Re: duplicate function oid symbols

    Andres Freund <andres@anarazel.de> — 2020-10-28T18:22:04Z

    Hi,

Thanks for fixing the HEAP_TABLE_AM_HANDLER_OID one.

On 2020-10-28 14:08:28 -0400, John Naylor wrote:
> > The only existing oid_symbol in pg_type that I think has enough
> > grandfather status to be tough to change is CASHOID for "money".
> > But we could imagine special-casing that with a handmade macro
> >
> > #define CASHOID MONEYOID
> >
> > and then getting rid of the oid_symbol entries.  (Or perhaps we
> > could just up and nuke CASHOID too?  It's somewhat dubious that
> > any outside code is really using that macro.)
> >
> 
> Yeah, grepping shows that some of those aren't even used in core code. On
> the other hand, the difference from the heap_am_handler case is the
> standard macros don't already exist for these pg_type entries. The handmade
> macro idea could be used for all eight just as easily as for one.

I think changing type oid macro names is somewhat problematic - in
contrast to function oid macros the type macros are much more likely to
be used by client applications, e.g. for deciding whether to use binary
or text format for a type.

A quick code search shows a few references, even just within debian
packages (some are incorrect hits, others aren't):
https://codesearch.debian.net/search?q=CASHOID&literal=1&perpkg=1

Greetings,

Andres Freund

  9. Re: duplicate function oid symbols

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-10-28T18:49:06Z

    Andres Freund <andres@anarazel.de> writes:
> I think changing type oid macro names is somewhat problematic - in
> contrast to function oid macros the type macros are much more likely to
> be used by client applications, e.g. for deciding whether to use binary
> or text format for a type.

> A quick code search shows a few references, even just within debian
> packages (some are incorrect hits, others aren't):
> https://codesearch.debian.net/search?q=CASHOID&literal=1&perpkg=1

Yeah, I can easily believe that for CASHOID in particular.  So I'm
okay with keeping that available as a handmade alias.  The other
extant oid_symbol entries are

PGNODETREEOID
PGNDISTINCTOID
PGDEPENDENCIESOID
PGMCVLISTOID
PGDDLCOMMANDOID
LSNOID
EVTTRIGGEROID

The only one of these that client code would plausibly be using is LSNOID,
and even that is a bit of a stretch.  Moreover, this clearly shows the
effect John mentioned that people have been copying the style of adjacent
entries rather than making use of the standard oid_symbol convention like
they should --- some of these don't exist in the initial v11 version of
pg_type.dat.

I'd suggest keeping CASHOID and LSNOID available as aliases, and renaming
the rest.

			regards, tom lane

  10. Re: duplicate function oid symbols

    Andres Freund <andres@anarazel.de> — 2020-10-28T19:17:12Z

    Hi,

On 2020-10-28 14:49:06 -0400, Tom Lane wrote:
> The other extant oid_symbol entries are
> 
> PGNODETREEOID
> PGNDISTINCTOID
> PGDEPENDENCIESOID
> PGMCVLISTOID
> PGDDLCOMMANDOID
> EVTTRIGGEROID

> The only one of these that client code would plausibly be using is LSNOID,
> and even that is a bit of a stretch.

There's a quite a few references to LSNOID in github code:
https://github.com/search?o=desc&q=LSNOID&s=indexed&type=Code

There also are a few references to the more marginal symbols above. But
they look more like somebody trying to be complete. E.g.
https://github.com/yugabyte/yugabyte-db/blob/8d0ef3f7f8c49a8d9bec302cdcc0c40f5d9e785b/src/postgres/src/backend/utils/misc/pg_yb_utils.c#L500
although there also is slightly more intentional looking references like
https://github.com/tada/pljava/blob/63d8a5e467a9c0f626c48e9ee134a58ac308fd8e/pljava/src/main/java/org/postgresql/pljava/jdbc/SQLXMLImpl.java#L177


> Moreover, this clearly shows the
> effect John mentioned that people have been copying the style of adjacent
> entries rather than making use of the standard oid_symbol convention like
> they should --- some of these don't exist in the initial v11 version of
> pg_type.dat.

Wonder if it's worth using something like 'backward_compat_oid_symbol'
and rejecting plain oid_symbol references for pg_type? That'd perhaps be
less likely to be copied?


> I'd suggest keeping CASHOID and LSNOID available as aliases, and renaming
> the rest.

I don't really have an opinion on wether it's worth keepign the other
aliases or not...

Greetings,

Andres Freund

  11. Re: duplicate function oid symbols

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-10-28T19:24:20Z

    Andres Freund <andres@anarazel.de> writes:
> On 2020-10-28 14:49:06 -0400, Tom Lane wrote:
>> Moreover, this clearly shows the
>> effect John mentioned that people have been copying the style of adjacent
>> entries rather than making use of the standard oid_symbol convention like
>> they should --- some of these don't exist in the initial v11 version of
>> pg_type.dat.

> Wonder if it's worth using something like 'backward_compat_oid_symbol'
> and rejecting plain oid_symbol references for pg_type? That'd perhaps be
> less likely to be copied?

Nah.  What I'm imagining is just that pg_type.h contains

#ifdef EXPOSE_TO_CLIENT_CODE

/*
 * Backwards compatibility for ancient random spellings of OID macros.
 * Don't use these macros in new code.
 */
#define CASHOID  MONEYOID
#define LSNOID   PG_LSNOID

#endif

and then the negotiation here is only about whether to make this list
longer.  We don't need to complicate genbki.pl with a new facility.

			regards, tom lane

  12. Re: duplicate function oid symbols

    Andres Freund <andres@anarazel.de> — 2020-10-28T19:28:40Z

    On 2020-10-28 15:24:20 -0400, Tom Lane wrote:
> Nah.  What I'm imagining is just that pg_type.h contains
> 
> #ifdef EXPOSE_TO_CLIENT_CODE
> 
> /*
>  * Backwards compatibility for ancient random spellings of OID macros.
>  * Don't use these macros in new code.
>  */
> #define CASHOID  MONEYOID
> #define LSNOID   PG_LSNOID
> 
> #endif

Ah, good idea. +1


> We don't need to complicate genbki.pl with a new facility.

I assume you plan to error out if oid_symbol is defined for pg_type
going forward?

  13. Re: duplicate function oid symbols

    John Naylor <john.naylor@enterprisedb.com> — 2020-10-28T19:28:49Z

    On Wed, Oct 28, 2020 at 3:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> and then the negotiation here is only about whether to make this list
> longer.  We don't need to complicate genbki.pl with a new facility.
>

Agreed, and reformat_dat_files.pl must also know about these special
attributes.

-- 
John Naylor
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  14. Re: duplicate function oid symbols

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-10-28T19:36:06Z

    Andres Freund <andres@anarazel.de> writes:
> I assume you plan to error out if oid_symbol is defined for pg_type
> going forward?

Right, just like we just did for pg_proc.

			regards, tom lane

  15. Re: duplicate function oid symbols

    John Naylor <john.naylor@enterprisedb.com> — 2020-10-28T19:58:33Z

    On Wed, Oct 28, 2020 at 3:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

>
> Nah.  What I'm imagining is just that pg_type.h contains
>
> #ifdef EXPOSE_TO_CLIENT_CODE
>
> /*
>  * Backwards compatibility for ancient random spellings of OID macros.
>  * Don't use these macros in new code.
>  */
> #define CASHOID  MONEYOID
> #define LSNOID   PG_LSNOID
>
> #endif
>

Here is a quick patch implementing this much.

-- 
John Naylor
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  16. Re: duplicate function oid symbols

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-10-29T17:35:08Z

    John Naylor <john.naylor@enterprisedb.com> writes:
> Here is a quick patch implementing this much.

Pushed with a couple cosmetic tweaks.

			regards, tom lane