Обсуждение: duplicate function oid symbols

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

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

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

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

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

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

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

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?

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

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

Pushed with a couple cosmetic tweaks.

            regards, tom lane