Thread: [HACKERS] many copies of atooid() and oid_cmp()
There are approximately 11 copies of atooid() and 3 of oid_cmp() or equivalent, and pending patches are proposing to add more. I propose these two patches to collect them in central places. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> There are approximately 11 copies of atooid() and 3 of oid_cmp() or
> equivalent, and pending patches are proposing to add more. I propose
> these two patches to collect them in central places.
+1 for the concept, but I'm a bit worried about putting atooid() in
postgres_ext.h. That's going to impose on the namespace of libpq-using
applications, for instance. A more conservative answer would be to
add it to c.h. OTOH, postgres_ext.h is where the Oid typedef lives,
so I do see the consistency of adding this there. Hard choice.
The oid_cmp() move looks fine if we only need it on the server side.
But doesn't pg_dump have one too?
regards, tom lane
On Wed, Jan 11, 2017 at 9:42 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > There are approximately 11 copies of atooid() and 3 of oid_cmp() or > equivalent, and pending patches are proposing to add more. I propose > these two patches to collect them in central places. > I've verified that the patch covers all the copies of atooid() and oid_cmp() that should be replaced. However, as Tom suggested, I've looked into pg_dump and found that there is a oidcmp() as well. It may have been missed because it has a different name. I was wondering whether it is worth to replace the following as well: (TransactionId)strtoul(str, NULL, 16) to something like #define atotranid(). -- Thanks & Regards, Kuntal Ghosh EnterpriseDB: http://www.enterprisedb.com
On 1/11/17 11:25 PM, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> There are approximately 11 copies of atooid() and 3 of oid_cmp() or >> equivalent, and pending patches are proposing to add more. I propose >> these two patches to collect them in central places. > > +1 for the concept, but I'm a bit worried about putting atooid() in > postgres_ext.h. That's going to impose on the namespace of libpq-using > applications, for instance. A more conservative answer would be to > add it to c.h. OTOH, postgres_ext.h is where the Oid typedef lives, > so I do see the consistency of adding this there. Hard choice. How about two copies: one in postgres_fe.h and one in postgres.h? > The oid_cmp() move looks fine if we only need it on the server side. > But doesn't pg_dump have one too? The pg_dump one isn't a qsort comparator, though. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 1/12/17 12:25 AM, Kuntal Ghosh wrote: > On Wed, Jan 11, 2017 at 9:42 PM, Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: >> There are approximately 11 copies of atooid() and 3 of oid_cmp() or >> equivalent, and pending patches are proposing to add more. I propose >> these two patches to collect them in central places. >> > I've verified that the patch covers all the copies of atooid() and > oid_cmp() that should be replaced. However, as Tom suggested, I've > looked into pg_dump and found that there is a oidcmp() as well. It may > have been missed because it has a different name. > > I was wondering whether it is worth to replace the following as well: > (TransactionId)strtoul(str, NULL, 16) to something like #define atotranid(). There is a atoxid(), which is actually not used and removed by my patch.There are a few other calls of this pattern, butthey are not frequent or consistent enough to worry about (yet), I think. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 1/11/17 11:25 PM, Tom Lane wrote:
>> +1 for the concept, but I'm a bit worried about putting atooid() in
>> postgres_ext.h. That's going to impose on the namespace of libpq-using
>> applications, for instance. A more conservative answer would be to
>> add it to c.h. OTOH, postgres_ext.h is where the Oid typedef lives,
>> so I do see the consistency of adding this there. Hard choice.
> How about two copies: one in postgres_fe.h and one in postgres.h?
That seems uglier than either of the other choices.
I don't personally have a huge problem with adding atooid in
postgres_ext.h, but I thought I'd better flag the potential issue
to see if anyone else thinks it's a big problem.
>> The oid_cmp() move looks fine if we only need it on the server side.
>> But doesn't pg_dump have one too?
> The pg_dump one isn't a qsort comparator, though.
OK.
regards, tom lane
On Thu, Jan 12, 2017 at 11:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> On 1/11/17 11:25 PM, Tom Lane wrote: >>> +1 for the concept, but I'm a bit worried about putting atooid() in >>> postgres_ext.h. That's going to impose on the namespace of libpq-using >>> applications, for instance. A more conservative answer would be to >>> add it to c.h. OTOH, postgres_ext.h is where the Oid typedef lives, >>> so I do see the consistency of adding this there. Hard choice. > >> How about two copies: one in postgres_fe.h and one in postgres.h? > > That seems uglier than either of the other choices. > > I don't personally have a huge problem with adding atooid in > postgres_ext.h, but I thought I'd better flag the potential issue > to see if anyone else thinks it's a big problem. FWIW, postgres_ext.h would make the most sense to me. Now, there is one way to not impose that to frontends linked to libpq which would be to locate it in some new header in src/include/common/, where both backend and frontend could reference to it. -- Michael
On 1/12/17 09:36, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> On 1/11/17 11:25 PM, Tom Lane wrote: >>> +1 for the concept, but I'm a bit worried about putting atooid() in >>> postgres_ext.h. That's going to impose on the namespace of libpq-using >>> applications, for instance. A more conservative answer would be to >>> add it to c.h. OTOH, postgres_ext.h is where the Oid typedef lives, >>> so I do see the consistency of adding this there. Hard choice. > >> How about two copies: one in postgres_fe.h and one in postgres.h? > > That seems uglier than either of the other choices. > > I don't personally have a huge problem with adding atooid in > postgres_ext.h, but I thought I'd better flag the potential issue > to see if anyone else thinks it's a big problem. committed as is then -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services