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 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

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

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

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

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

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&amp;literal=1&amp;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&amp;literal=1&amp;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&amp;q=LSNOID&amp;s=indexed&amp;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?

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

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

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

John Naylor <john.naylor@enterprisedb.com> writes:

Here is a quick patch implementing this much.

Pushed with a couple cosmetic tweaks.

regards, tom lane