Horton

Thread

Commits

  1. Remove remaining hard-wired OID references in the initial catalog data.

  2. Create a script that can renumber manually-assigned OIDs.

  3. Minor improvements for reformat_dat_file.pl.

  1. Why don't we have a small reserved OID range for patch revisions?

    Peter Geoghegan <pg@bowt.ie> — 2019-02-08T17:59:42Z

    Why don't we provide a small reserved OID range that can be used by
patch authors temporarily, with the expectation that they'll be
replaced by "real" OIDs at the point the patch gets committed? This
would be similar the situation with catversion bumps -- we don't
expect patches that will eventually need them to have them.

It's considered good practice to choose an OID that's at the beginning
of the range shown by the unused_oids script, so naturally there is a
good chance that any patch that adds a system catalog entry will bit
rot prematurely. This seems totally unnecessary to me. You could even
have a replace_oids script under this system. That would replace the
known-temporary OIDs with mapped contiguous real values at the time of
commit (maybe it would just print out which permanent OIDs to use in
place of the temporary ones, and leave the rest up to the committer).
I don't do Perl, so I'm not volunteering for this.

-- 
Peter Geoghegan

  2. Re: Why don't we have a small reserved OID range for patch revisions?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-08T18:14:15Z

    Peter Geoghegan <pg@bowt.ie> writes:
> Why don't we provide a small reserved OID range that can be used by
> patch authors temporarily, with the expectation that they'll be
> replaced by "real" OIDs at the point the patch gets committed? This
> would be similar the situation with catversion bumps -- we don't
> expect patches that will eventually need them to have them.

Quite a few people have used OIDs up around 8000 or 9000 for this purpose;
I doubt we need a formally reserved range for it.  The main problem with
doing it is the hazard that the patch'll get committed just like that,
suddenly breaking things for everyone else doing likewise.

(I would argue, in fact, that the reason we have any preassigned OIDs
above perhaps 6000 is that exactly this has happened before.)

A script such as you suggest might be a good way to reduce the temptation
to get lazy at the last minute.  Now that the catalog data is pretty
machine-readable, I suspect it wouldn't be very hard --- though I'm
not volunteering either.  I'm envisioning something simple like "renumber
all OIDs in range mmmm-nnnn into range xxxx-yyyy", perhaps with the
ability to skip any already-used OIDs in the target range.

			regards, tom lane

  3. Re: Why don't we have a small reserved OID range for patch revisions?

    Peter Geoghegan <pg@bowt.ie> — 2019-02-08T18:19:41Z

    On Fri, Feb 8, 2019 at 10:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> A script such as you suggest might be a good way to reduce the temptation
> to get lazy at the last minute.  Now that the catalog data is pretty
> machine-readable, I suspect it wouldn't be very hard --- though I'm
> not volunteering either.  I'm envisioning something simple like "renumber
> all OIDs in range mmmm-nnnn into range xxxx-yyyy", perhaps with the
> ability to skip any already-used OIDs in the target range.

I imagined that the machine-readable catalog data would allow us to
assign non-numeric identifiers to this OID range. Perhaps there'd be a
textual symbol with a number in the range of 0-20 at the end. Those
would stick out like a sore thumb, making it highly unlikely that
anybody would forget about it at the last minute.

-- 
Peter Geoghegan

  4. Re: Why don't we have a small reserved OID range for patch revisions?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-08T18:29:20Z

    Peter Geoghegan <pg@bowt.ie> writes:
> On Fri, Feb 8, 2019 at 10:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> A script such as you suggest might be a good way to reduce the temptation
>> to get lazy at the last minute.  Now that the catalog data is pretty
>> machine-readable, I suspect it wouldn't be very hard --- though I'm
>> not volunteering either.  I'm envisioning something simple like "renumber
>> all OIDs in range mmmm-nnnn into range xxxx-yyyy", perhaps with the
>> ability to skip any already-used OIDs in the target range.

> I imagined that the machine-readable catalog data would allow us to
> assign non-numeric identifiers to this OID range. Perhaps there'd be a
> textual symbol with a number in the range of 0-20 at the end. Those
> would stick out like a sore thumb, making it highly unlikely that
> anybody would forget about it at the last minute.

Um.  That would not be just an add-on script but something that
genbki.pl would have to accept.  I'm not excited about that; it would
complicate what's already complex, and if it works enough for test
purposes then it wouldn't really stop a committer who wasn't paying
attention from committing the patch un-revised.

To the extent that this works at all, OIDs in the 9000 range ought
to be enough of a flag already, I think.

			regards, tom lane

  5. Re: Why don't we have a small reserved OID range for patch revisions?

    Peter Geoghegan <pg@bowt.ie> — 2019-02-08T18:35:14Z

    On Fri, Feb 8, 2019 at 10:29 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Um.  That would not be just an add-on script but something that
> genbki.pl would have to accept.  I'm not excited about that; it would
> complicate what's already complex, and if it works enough for test
> purposes then it wouldn't really stop a committer who wasn't paying
> attention from committing the patch un-revised.
>
> To the extent that this works at all, OIDs in the 9000 range ought
> to be enough of a flag already, I think.

I tend to agree that this isn't enough of a problem to justify making
genbki.pl significantly more complicated.

-- 
Peter Geoghegan

  6. Re: Why don't we have a small reserved OID range for patch revisions?

    Robert Haas <robertmhaas@gmail.com> — 2019-02-09T09:10:52Z

    On Fri, Feb 8, 2019 at 11:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> To the extent that this works at all, OIDs in the 9000 range ought
> to be enough of a flag already, I think.

A "flag" that isn't documented anywhere outside of a mailing list
discussion and that isn't checked by any code anywhere is not much of
a flag, IMHO.

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

  7. Re: Why don't we have a small reserved OID range for patch revisions?

    John Naylor <john.naylor@2ndquadrant.com> — 2019-02-11T11:44:39Z

    On 2/8/19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> A script such as you suggest might be a good way to reduce the temptation
> to get lazy at the last minute.  Now that the catalog data is pretty
> machine-readable, I suspect it wouldn't be very hard --- though I'm
> not volunteering either.  I'm envisioning something simple like "renumber
> all OIDs in range mmmm-nnnn into range xxxx-yyyy", perhaps with the
> ability to skip any already-used OIDs in the target range.

This might be something that can be done inside reformat_dat_files.pl.
It's a little outside it's scope, but better than the alternatives.
And we already have a function in Catalog.pm to get the currently used
oids. I'll volunteer to look into it but I don't know when that will
be.

-- 
John Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  8. Re: Why don't we have a small reserved OID range for patch revisions?

    John Naylor <john.naylor@2ndquadrant.com> — 2019-02-14T16:01:35Z

    I wrote:

> On 2/8/19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> A script such as you suggest might be a good way to reduce the temptation
>> to get lazy at the last minute.  Now that the catalog data is pretty
>> machine-readable, I suspect it wouldn't be very hard --- though I'm
>> not volunteering either.  I'm envisioning something simple like "renumber
>> all OIDs in range mmmm-nnnn into range xxxx-yyyy", perhaps with the
>> ability to skip any already-used OIDs in the target range.
>
> This might be something that can be done inside reformat_dat_files.pl.
> It's a little outside it's scope, but better than the alternatives.

Along those lines, here's a draft patch to do just that. It handles
array type oids as well. Run it like this:

perl  reformat_dat_file.pl  --map-from 9000  --map-to 2000  *.dat

There is some attempt at documentation. So far it doesn't map by
default, but that could be changed if we agreed on the convention of
9000 or whatever.

-- 
John Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  9. Re: Why don't we have a small reserved OID range for patch revisions?

    John Naylor <john.naylor@2ndquadrant.com> — 2019-02-23T08:13:21Z

    I wrote:

> Along those lines, here's a draft patch to do just that. It handles
> array type oids as well. Run it like this:
>
> perl  reformat_dat_file.pl  --map-from 9000  --map-to 2000  *.dat
>
> There is some attempt at documentation. So far it doesn't map by
> default, but that could be changed if we agreed on the convention of
> 9000 or whatever.

In case we don't want to lose track of this, I added it to the March
commitfest with a target of v13. (I didn't see a way to  add it to the
July commitfest)

-- 
John Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  10. Re: Why don't we have a small reserved OID range for patch revisions?

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-02-27T09:59:00Z

    On 2019-02-08 19:14, Tom Lane wrote:
> Quite a few people have used OIDs up around 8000 or 9000 for this purpose;
> I doubt we need a formally reserved range for it.  The main problem with
> doing it is the hazard that the patch'll get committed just like that,
> suddenly breaking things for everyone else doing likewise.

For that reason, I'm not in favor of this.  Forgetting to update the
catversion is already common enough (for me).  Adding another step
between having a seemingly final patch and being able to actually commit
it doesn't seem attractive.  Moreover, these "final adjustments" would
tend to require a full rebuild and retest, adding even more overhead.

OID collision doesn't seem to be a significant problem (for me).

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

  11. Re: Why don't we have a small reserved OID range for patch revisions?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-27T21:27:04Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2019-02-08 19:14, Tom Lane wrote:
>> Quite a few people have used OIDs up around 8000 or 9000 for this purpose;
>> I doubt we need a formally reserved range for it.  The main problem with
>> doing it is the hazard that the patch'll get committed just like that,
>> suddenly breaking things for everyone else doing likewise.

> For that reason, I'm not in favor of this.  Forgetting to update the
> catversion is already common enough (for me).  Adding another step
> between having a seemingly final patch and being able to actually commit
> it doesn't seem attractive.  Moreover, these "final adjustments" would
> tend to require a full rebuild and retest, adding even more overhead.

> OID collision doesn't seem to be a significant problem (for me).

Um, I beg to differ.  It's not at all unusual for pending patches to
bit-rot for no reason other than suddenly getting an OID conflict.
I don't have to look far for a current example:
https://travis-ci.org/postgresql-cfbot/postgresql/builds/498955351

We do need a couple of pieces of new infrastructure to make this idea
conveniently workable.  One is a tool to allow automatic OID renumbering
instead of having to do it by hand; Naylor has a draft for that upthread.

Perhaps it'd be useful for genbki.pl to spit out a warning (NOT an
error) if it sees OIDs in the reserved range.  I'm not sure that that'd
really be worth the trouble though, since one could easily forget
about it while reviewing/testing just before commit, and it'd just be
useless noise up until it was time to commit.

Another issue, as Robert pointed out, is that this does need to be
a formal convention not something undocumented.  Naylor's patch adds
a mention of it in bki.sgml, but I wonder if anyplace else should
talk about it.

I concede your point that a prudent committer would do a rebuild and
retest rather than just trusting the tool.  But really, how much
extra work is that?  If you've spent any time optimizing your workflow,
a full rebuild and check-world should be under five minutes on any
hardware anyone would be using for development today.

And, yeah, we probably will make mistakes like this, just like we
sometimes forget the catversion bump.  As long as we have a tool
for OID renumbering, I don't think that's the end of the world.
Fixing it after the fact isn't going to be a big deal, any more
than it is for catversion.

			regards, tom lane

  12. Re: Why don't we have a small reserved OID range for patch revisions?

    Peter Geoghegan <pg@bowt.ie> — 2019-02-27T21:50:35Z

    On Wed, Feb 27, 2019 at 1:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > OID collision doesn't seem to be a significant problem (for me).
>
> Um, I beg to differ.  It's not at all unusual for pending patches to
> bit-rot for no reason other than suddenly getting an OID conflict.
> I don't have to look far for a current example:
> https://travis-ci.org/postgresql-cfbot/postgresql/builds/498955351

OID conflicts are not that big of a deal when building a patch
locally, because almost everyone knows what the exact problem is
immediately, and because you probably have more than a passing
interest in the patch to even do that much. However, the continuous
integration stuff has created an expectation that your patch shouldn't
be left to bitrot for long. Silly mechanical bitrot now seems like a
much bigger problem than it was before these developments. It unfairly
puts reviewers off engaging.

Patch authors shouldn't be left with any excuse for leaving their
patch to bitrot for long. And, more casual patch reviewers shouldn't
have any excuse for not downloading a patch and applying it locally,
so that they can spend a spare 10 minutes kicking the tires.

> Perhaps it'd be useful for genbki.pl to spit out a warning (NOT an
> error) if it sees OIDs in the reserved range.  I'm not sure that that'd
> really be worth the trouble though, since one could easily forget
> about it while reviewing/testing just before commit, and it'd just be
> useless noise up until it was time to commit.

My sense is that we should err on the side of being informative.

> Another issue, as Robert pointed out, is that this does need to be
> a formal convention not something undocumented.  Naylor's patch adds
> a mention of it in bki.sgml, but I wonder if anyplace else should
> talk about it.

Why not have unused_oids reference the convention as a "tip"?

> I concede your point that a prudent committer would do a rebuild and
> retest rather than just trusting the tool.  But really, how much
> extra work is that?  If you've spent any time optimizing your workflow,
> a full rebuild and check-world should be under five minutes on any
> hardware anyone would be using for development today.

If you use the "check-world parallel" recipe on the committing
checklist Wiki page, and if you use ccache, ~2 minutes is attainable
for optimized builds (though the recipe doesn't work on all release
branches). I don't think that a committer should be a committing
anything if they're not willing to do this much. It's not just prudent
-- it is the *bare minimum* when committing a patch that creates
system catalog entries.

-- 
Peter Geoghegan

  13. Re: Why don't we have a small reserved OID range for patch revisions?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-27T21:59:17Z

    I wrote:
> We do need a couple of pieces of new infrastructure to make this idea
> conveniently workable.  One is a tool to allow automatic OID renumbering
> instead of having to do it by hand; Naylor has a draft for that upthread.

Oh: arguably, something else we'd need to do to ensure that OID
renumbering is trouble-free is to institute a strict rule that OID
references in the *.dat files must be symbolic.  We had not bothered
to convert every single reference type before, reasoning that some
of them were too little-used to be worth the trouble; but someday
that'll rise up to bite us, if semi-automated renumbering becomes
a thing.

It looks to me like the following OID columns remain unconverted:

pg_class.reltype
pg_database.dattablespace
pg_ts_config.cfgparser
pg_ts_config_map.mapcfg, mapdict
pg_ts_dict.dicttemplate
pg_type.typcollation
pg_type.typrelid

			regards, tom lane

  14. Re: Why don't we have a small reserved OID range for patch revisions?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-27T22:09:42Z

    Peter Geoghegan <pg@bowt.ie> writes:
> On Wed, Feb 27, 2019 at 1:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> OID collision doesn't seem to be a significant problem (for me).

>> Um, I beg to differ.  It's not at all unusual for pending patches to
>> bit-rot for no reason other than suddenly getting an OID conflict.
>> I don't have to look far for a current example:
>> https://travis-ci.org/postgresql-cfbot/postgresql/builds/498955351

> Patch authors shouldn't be left with any excuse for leaving their
> patch to bitrot for long. And, more casual patch reviewers shouldn't
> have any excuse for not downloading a patch and applying it locally,
> so that they can spend a spare 10 minutes kicking the tires.

Yeah, that latter point is really the killer argument.  We don't want
to make people spend valuable review time on fixing uninteresting OID
conflicts.  It's even more annoying that several people might have to
duplicate the same work, if they're testing a patch independently.

Given a convention that under-development patches use OIDs in the 9K
range, the only time anybody would have to resolve OID conflicts for
testing would be if they were trying to test the combination of two
or more patches.  Even then, an OID-renumbering script would make it
pretty painless: apply patch 1, renumber its OIDs to someplace else,
apply patch 2, repeat as needed.

> Why not have unused_oids reference the convention as a "tip"?

Hmm, could be helpful.

			regards, tom lane

  15. Re: Why don't we have a small reserved OID range for patch revisions?

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-02-27T22:38:58Z

    On 2019-02-27 22:27, Tom Lane wrote:
>> OID collision doesn't seem to be a significant problem (for me).
> 
> Um, I beg to differ.  It's not at all unusual for pending patches to
> bit-rot for no reason other than suddenly getting an OID conflict.
> I don't have to look far for a current example:

I'm not saying it doesn't happen, but that it's not a significant
problem overall.

The changes of a patch (a) allocating a new OID, (b) a second patch
allocating a new OID, (c) both being in flight at the same time, (d)
actually picking the same OID, are small.  I guess the overall time lost
to this issue is perhaps 2 hours per year.  On the other hand, with
about 2000 commits to master per year, if this renumbering business only
adds 2 seconds of overhead to committing, we're coming out behind.

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

  16. Re: Why don't we have a small reserved OID range for patch revisions?

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-02-27T22:44:30Z

    On 2019-02-27 22:50, Peter Geoghegan wrote:
> However, the continuous
> integration stuff has created an expectation that your patch shouldn't
> be left to bitrot for long. Silly mechanical bitrot now seems like a
> much bigger problem than it was before these developments. It unfairly
> puts reviewers off engaging.

If this is the problem (although I think we'd find that OID collisions
are rather rare compared to other gratuitous cfbot failures), why not
have the cfbot build with a flag that ignores OID collisions?

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

  17. Re: Why don't we have a small reserved OID range for patch revisions?

    Peter Geoghegan <pg@bowt.ie> — 2019-02-27T22:44:32Z

    On Wed, Feb 27, 2019 at 2:39 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> The changes of a patch (a) allocating a new OID, (b) a second patch
> allocating a new OID, (c) both being in flight at the same time, (d)
> actually picking the same OID, are small.

But...they are. Most patches don't create new system catalog entries
at all. Of those that do, the conventions around assigning new OIDs
make it fairly likely that problems will emerge.

> I guess the overall time lost
> to this issue is perhaps 2 hours per year.  On the other hand, with
> about 2000 commits to master per year, if this renumbering business only
> adds 2 seconds of overhead to committing, we're coming out behind.

The time spent on the final commit is not the cost we're concerned
about, though. It isn't necessary to do that more than once, whereas
all but the most trivial of patches receive multiple rounds of review
and revision.

-- 
Peter Geoghegan

  18. Re: Why don't we have a small reserved OID range for patch revisions?

    Peter Geoghegan <pg@bowt.ie> — 2019-02-27T22:45:14Z

    On Wed, Feb 27, 2019 at 2:44 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> If this is the problem (although I think we'd find that OID collisions
> are rather rare compared to other gratuitous cfbot failures), why not
> have the cfbot build with a flag that ignores OID collisions?

How would that work?

-- 
Peter Geoghegan

  19. Re: Why don't we have a small reserved OID range for patch revisions?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-27T22:57:56Z

    Peter Geoghegan <pg@bowt.ie> writes:
> On Wed, Feb 27, 2019 at 2:44 PM Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> If this is the problem (although I think we'd find that OID collisions
>> are rather rare compared to other gratuitous cfbot failures), why not
>> have the cfbot build with a flag that ignores OID collisions?

> How would that work?

It could work for conflicting OIDs in different system catalogs (so that
the "conflict" is an artifact of our assignment rules rather than an
intrinsic problem).  But I think the majority of new hand-assigned OIDs
are in pg_proc, so that this kind of hack would not help as much as one
might wish.

			regards, tom lane

  20. Re: Why don't we have a small reserved OID range for patch revisions?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-28T03:41:20Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2019-02-27 22:27, Tom Lane wrote:
>>> OID collision doesn't seem to be a significant problem (for me).

>> Um, I beg to differ.  It's not at all unusual for pending patches to
>> bit-rot for no reason other than suddenly getting an OID conflict.
>> I don't have to look far for a current example:

> I'm not saying it doesn't happen, but that it's not a significant
> problem overall.

I do not think you are correct.  It may not be a big problem across
all our incoming patches, but that's only because most of them don't
have anything to do with hand-assigned OIDs.  Among those that do,
I think there is a significant problem.

To try to quantify this a bit, I looked through v12-cycle and pending
patches that touch the catalog data.

We've only committed 12 patches adding new hand-assigned OIDs since v11
was branched off.  (I suspect that's lower than in a typical cycle,
but have not attempted to quantify things further back.)  Of those,
only two seem to have needed OID adjustments after initial posting,
but that's mostly because most of them were committer-originated patches
that got pushed within a week or two.  That's certainly not the typical
wait time for a patch submitted by anybody else.  Also, a lot of these
patches recycled OIDs that'd recently been freed by patches such as the
abstime-ectomy, which means that the amount of OID conflict created for
pending patches is probably *really* low in this cycle-so-far, compared
to our historical norms.

Of what's in the queue to be reviewed right now, there are just
20 (out of 150-plus) patches that touch the catalog/*.dat files.
I got this number by groveling through the cfbot's reports of
patch applications, to see which patches touched those files.
It might omit some patches that the cfbot failed to make sense of.
Also, I'm pretty sure that a few of these patches don't actually
assign any new OIDs but just change existing entries, or create
only entries with auto-assigned OIDs.  I did not try to separate
those out, however, since the point here is to estimate for how
many patches a committer would even need to think about this.

Of those twenty patches, three have unresolved OID conflicts
right now:

multivariate MCV lists and histograms
commontype polymorphics
log10/hyper functions

Another one has recently had to resolve an OID conflict:

SortSupport implementation on inet/cdir	

which is notable considering that that thread is less than three weeks
old.  (The log10 and commontype threads aren't really ancient either.)

I spent some extra effort looking at the patches that both create more
than a few new OIDs and have been around for awhile:

Generic type subscripting
KNN for btree
Custom compression methods
SQL/JSON: functions
SQL/JSON: jsonpath
Generated columns
BRIN bloom indexes

The first four of those have all had to reassign OIDs during their
lifetime.  jsonpath has avoided doing so by choosing fairly high-numbered
OIDs (6K range) to begin with; which I trust you will agree is a solution
that doesn't scale for long.  I'm not entirely sure that the last two
haven't had to renumber OIDs; I ran out of energy before poking through
their history in detail.

In short, this situation may look fine from the perspective of a committer
with a relatively short timeline to commit, but it's pretty darn awful for
everybody else.  The only way to avoid a ~ 50% failure rate is to choose
OIDs above 6K, and once everybody starts doing it like that, things are
going to get very unpleasant very quickly.

			regards, tom lane

  21. Re: Why don't we have a small reserved OID range for patch revisions?

    Robert Haas <robertmhaas@gmail.com> — 2019-02-28T13:59:58Z

    On Wed, Feb 27, 2019 at 10:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> In short, this situation may look fine from the perspective of a committer
> with a relatively short timeline to commit, but it's pretty darn awful for
> everybody else.  The only way to avoid a ~ 50% failure rate is to choose
> OIDs above 6K, and once everybody starts doing it like that, things are
> going to get very unpleasant very quickly.

The root problem here from the perspective of a non-committer is not
that they might have to renumber OIDs a few times over the year or two
it takes to get their patch merged, but rather that it takes a year or
two to get their patch merged.  That's not to say that I have no
sympathy with people in that situation or don't want to make their
lives easier, but I'm not really convinced that burdening committers
with additional manual steps is the right way to get patches merged
faster.  This seems like a big piece of new mechanism being invented
to solve an occasional annoyance. Your statistics are not convincing
at all: you're arguing that this is a big problem because 2-3% of
pending patches current have an issue here, and some others have in
the past, but that's a really small percentage, and the time spent
doing OID renumbering must be a tiny percentage of the total time
anyone spends hacking on PostgreSQL.

I think that the problem here is have a very limited range of OIDs
(10k) which can be used for this purpose, and the number of OIDs that
are used in that space is now a significant fraction of the total
(>4.5k), and the problem is further complicated by the desire to keep
the OIDs assigned near the low end of the available numbering space
and/or near to other OIDs used for similar purposes.  The sheer fact
that the address space is nearly half-used means that conflicts are
likely even if people choose OIDs at random, and when people choose
OIDs non-randomly -- lowest, round numbers, near to other OIDs -- the
chances of conflicts just go up.

We could fix that problem by caring less about keeping all the numbers
gapless and increasing the size of the reserved space to say 100k, but
just as a thought, what if we stopped assigning manual OIDs for new
catalog entries altogether, except for once at the end of each release
cycle?  Make a way that people can add an entry to pg_proc.dat or
whatever without fixing an OID, and let the build scripts generate
one.  As many patches as happen during a release cycle will add new
such entries and they'll just all get some OID or other.  Then, at the
end of the release cycle, we'll run a script that finds all of those
catalog entries and rewrites the .dat files, adding a permanent OID
assignment to each one, so that those OIDs will then be fixed for all
future releases (unless we drop the entries or explicitly change
something).

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

  22. Re: Why don't we have a small reserved OID range for patch revisions?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-28T15:27:40Z

    Robert Haas <robertmhaas@gmail.com> writes:
> This seems like a big piece of new mechanism being invented
> to solve an occasional annoyance. Your statistics are not convincing
> at all: you're arguing that this is a big problem because 2-3% of
> pending patches current have an issue here, and some others have in
> the past, but that's a really small percentage, and the time spent
> doing OID renumbering must be a tiny percentage of the total time
> anyone spends hacking on PostgreSQL.

TBH, I find this position utterly baffling.  It's true that only a
small percentage of patches have an issue here, because only a small
percentage of patches dabble in manually-assigned OIDs at all.  But
*among those that do*, there is a huge problem.  I had not actually
realized how bad it is until I gathered those stats, but it's bad.

I don't understand the objection to inventing a mechanism that will
help those patches and has no impact whatever when working on patches
that don't involve manually-assigned OIDs.

And, yeah, I'd like us not to have patches hanging around for years
either, but that's a reality that's not going away.

> We could fix that problem by caring less about keeping all the numbers
> gapless and increasing the size of the reserved space to say 100k,

We already had this discussion.  Moving FirstNormalObjectId is infeasible
without forcing a dump/reload, which I don't think anyone wants to do.

> but
> just as a thought, what if we stopped assigning manual OIDs for new
> catalog entries altogether, except for once at the end of each release
> cycle?

And that's another idea without any basis in reality.  What are you
going to do instead?  What mechanism will you use to track these
OIDs so you can clean up later?  Who's going to write the code that
will support this?  Not me.  I think the proposal that is on the
table is superior.

			regards, tom lane

  23. Re: Why don't we have a small reserved OID range for patch revisions?

    Robert Haas <robertmhaas@gmail.com> — 2019-02-28T15:58:49Z

    On Thu, Feb 28, 2019 at 10:27 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > This seems like a big piece of new mechanism being invented
> > to solve an occasional annoyance. Your statistics are not convincing
> > at all: you're arguing that this is a big problem because 2-3% of
> > pending patches current have an issue here, and some others have in
> > the past, but that's a really small percentage, and the time spent
> > doing OID renumbering must be a tiny percentage of the total time
> > anyone spends hacking on PostgreSQL.
>
> TBH, I find this position utterly baffling.  It's true that only a
> small percentage of patches have an issue here, because only a small
> percentage of patches dabble in manually-assigned OIDs at all.  But
> *among those that do*, there is a huge problem.  I had not actually
> realized how bad it is until I gathered those stats, but it's bad.
>
> I don't understand the objection to inventing a mechanism that will
> help those patches and has no impact whatever when working on patches
> that don't involve manually-assigned OIDs.
>
> And, yeah, I'd like us not to have patches hanging around for years
> either, but that's a reality that's not going away.

I don't think this is the worst proposal ever.  However, I also think
that it's not unreasonable to raise the issue that writing OR
reviewing OR committing a patch already involves adhering to a thicket
of undocumented rules.  When somebody fails to adhere to one of those
rules, they get ignored or publicly shamed.  Now you want to add yet
another step to the process - really two.  If you want to submit a
patch that requires new catalog entries, you must know that you're
supposed to put those OIDs in this new range that we're going to set
aside for such things, and if you want to commit one, you must know
that you're suppose to renumber those OID assignments into some other
range.  And people are going to screw it up - submitters are going to
fail to know about this new policy (which will probably be documented
nowhere, just like all the other ones) - and committers are going to
fail to remember to renumber things.  So, I suspect that for every
unit of work it saves somebody, it's probably going to generate about
one unit of extra work for somebody else.

A lot of projects have a much less painful process for getting patches
integrated than we do.  I don't know how those projects maintain
adequate code quality, but I do know that making it easy to get a
patch accepted makes people more likely to contribute patches, and
increases overall development velocity.  It is not even vaguely
unreasonable to worry about whether making this more complicated is
going to hurt more than it helps, and I don't know why you think
otherwise.

> > but
> > just as a thought, what if we stopped assigning manual OIDs for new
> > catalog entries altogether, except for once at the end of each release
> > cycle?
>
> And that's another idea without any basis in reality.  What are you
> going to do instead?  What mechanism will you use to track these
> OIDs so you can clean up later?

Right now every entry in pg_proc.dat includes an OID assignment.  What
I'm proposing is that we would also allow entries that did not have
one, and the build process would assign one while processing the .dat
files.  Then later, somebody could use a script that went through and
rewrote the .dat file to add OID assignments to any entries that
lacked them.  Since the topic of having tools for automated rewrite of
those files has been discussed at length, and since we already have a
script called reformat_dat_file.pl in the tree which  contains
comments indicating that it could be modified for bulk editing, said
script having been committed BY YOU, I don't understand why you think
that bulk editing is infeasible.

> Who's going to write the code that
> will support this?  Not me.  I think the proposal that is on the
> table is superior.

OK.  Well, I think that doing nothing is superior to this proposal,
for reasons similar to what Peter Eisentraut has already articulated.
And I think rather than blasting forward with your own preferred
alternative in the face of disagreement, you should be willing to
discuss other possible options.  But if you're not willing to do that,
I can't make you.

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

  24. Re: Why don't we have a small reserved OID range for patch revisions?

    Peter Geoghegan <pg@bowt.ie> — 2019-02-28T22:36:16Z

    On Thu, Feb 28, 2019 at 7:59 AM Robert Haas <robertmhaas@gmail.com> wrote:
> I don't think this is the worst proposal ever.  However, I also think
> that it's not unreasonable to raise the issue that writing OR
> reviewing OR committing a patch already involves adhering to a thicket
> of undocumented rules.  When somebody fails to adhere to one of those
> rules, they get ignored or publicly shamed.  Now you want to add yet
> another step to the process - really two.

There does seem to be a real problem with undocumented processes. For
example, I must confess that it came as news to me that we already had
a reserved OID range. However, I don't think that there is that much
of an issue with adding new mechanisms like this, provided it makes it
easy to do the right thing and hard to do the wrong thing. What Tom
has proposed so far is not something that self-evidently meets that
standard, but it's also not something that self-evidently fails to
meet that standard.

I have attempted to institute some general guidelines for what the
thicket of rules are by creating the "committing checklist" page. This
is necessarily imperfect, because the rules are in many cases open to
interpretation, often for good practical reasons. I don't have any
sympathy for committers that find it hard to remember to do a
catversion bump with any kind of regularity. That complexity seems
inherent, not incidental, since it's often convenient to ignore
catalog incompatibilities during development.

> So, I suspect that for every
> unit of work it saves somebody, it's probably going to generate about
> one unit of extra work for somebody else.

Maybe so. I think that you're jumping to conclusions, though.

> A lot of projects have a much less painful process for getting patches
> integrated than we do.  I don't know how those projects maintain
> adequate code quality, but I do know that making it easy to get a
> patch accepted makes people more likely to contribute patches, and
> increases overall development velocity.  It is not even vaguely
> unreasonable to worry about whether making this more complicated is
> going to hurt more than it helps, and I don't know why you think
> otherwise.

But you seem to want to make the mechanism itself even more
complicated, not less complicated (based on your remarks about making
OID assignment happen during the build). In order to make the use of
the mechanism easier. That seems worth considering, but ISTM that this
is talking at cross purposes. There are far simpler ways of making it
unlikely that a committer is going to miss this step. There is also a
simple way of noticing that they do quickly (e.g. a simple buildfarm
test).

> Right now every entry in pg_proc.dat includes an OID assignment.  What
> I'm proposing is that we would also allow entries that did not have
> one, and the build process would assign one while processing the .dat
> files.  Then later, somebody could use a script that went through and
> rewrote the .dat file to add OID assignments to any entries that
> lacked them.  Since the topic of having tools for automated rewrite of
> those files has been discussed at length, and since we already have a
> script called reformat_dat_file.pl in the tree which  contains
> comments indicating that it could be modified for bulk editing, said
> script having been committed BY YOU, I don't understand why you think
> that bulk editing is infeasible.

I'm also curious to hear what Tom thinks about this.

> OK.  Well, I think that doing nothing is superior to this proposal,
> for reasons similar to what Peter Eisentraut has already articulated.
> And I think rather than blasting forward with your own preferred
> alternative in the face of disagreement, you should be willing to
> discuss other possible options.  But if you're not willing to do that,
> I can't make you.

Peter seemed to not want to do this on the grounds that it isn't
necessary at all, whereas you think that it doesn't go far enough. If
there is a consensus against what Tom has said, it's a cacophonous one
that cannot really be said to be in favor of anything.

-- 
Peter Geoghegan

  25. Re: Why don't we have a small reserved OID range for patch revisions?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-28T23:09:41Z

    Peter Geoghegan <pg@bowt.ie> writes:
> On Thu, Feb 28, 2019 at 7:59 AM Robert Haas <robertmhaas@gmail.com> wrote:
>> OK.  Well, I think that doing nothing is superior to this proposal,
>> for reasons similar to what Peter Eisentraut has already articulated.
>> And I think rather than blasting forward with your own preferred
>> alternative in the face of disagreement, you should be willing to
>> discuss other possible options.  But if you're not willing to do that,
>> I can't make you.

> Peter seemed to not want to do this on the grounds that it isn't
> necessary at all, whereas you think that it doesn't go far enough. If
> there is a consensus against what Tom has said, it's a cacophonous one
> that cannot really be said to be in favor of anything.

The only thing that's really clear is that some senior committers don't
want to be bothered because they don't think there's a problem here that
justifies any additional expenditure of their time.  Perhaps they are
right, because I'd expected some comments from non-committer developers
confirming that they see a problem, and the silence is deafening.

I'm inclined to commit some form of Naylor's tool improvement anyway,
because I have use for it; I remember times when I've renumbered OIDs
manually in patches, and it wasn't much fun.  But I can't force a
process change if there's not consensus for it among the committers.

			regards, tom lane

  26. Re: Why don't we have a small reserved OID range for patch revisions?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-28T23:40:24Z

    Robert Haas <robertmhaas@gmail.com> writes:
>>> just as a thought, what if we stopped assigning manual OIDs for new
>>> catalog entries altogether, except for once at the end of each release
>>> cycle?

Actually ... that leads to an idea that wouldn't add any per-commit
overhead, or really much change at all to existing processes.  Given
the existence of a reliable OID-renumbering tool, we could:

1. Encourage people to develop new patches using chosen-at-random
high OIDs, in the 7K-9K range.  They do this already, it'd just
be encouraged instead of discouraged.

2. Commit patches as received.

3. Once each devel cycle, after feature freeze, somebody uses the
renumbering tool to shove all the new OIDs down to lower numbers,
freeing the high-OID range for the next devel cycle.  We'd have
to remember to do that, but it could be added to the RELEASE_CHANGES
checklist.

In this scheme, OID collisions are a problem for in-progress patches
only if two patches are unlucky enough to choose the same random
high OIDs during the same devel cycle.  That's unlikely, or at least
a good bit less likely than collisions are today.  If/when it does
happen we'd have a couple of alternatives for ameliorating the problem
--- either the not-yet-committed patch could use the renumbering tool
on their own OIDs, or we could do an off-schedule run of step 3 to get
the already-committed OIDs out of their way.

			regards, tom lane

  27. Re: Why don't we have a small reserved OID range for patch revisions?

    Peter Geoghegan <pg@bowt.ie> — 2019-02-28T23:41:18Z

    On Thu, Feb 28, 2019 at 3:09 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The only thing that's really clear is that some senior committers don't
> want to be bothered because they don't think there's a problem here that
> justifies any additional expenditure of their time.  Perhaps they are
> right, because I'd expected some comments from non-committer developers
> confirming that they see a problem, and the silence is deafening.

I don't think that you can take that as too strong a signal. The
incentives are different for non-committers.

> I'm inclined to commit some form of Naylor's tool improvement anyway,
> because I have use for it; I remember times when I've renumbered OIDs
> manually in patches, and it wasn't much fun.  But I can't force a
> process change if there's not consensus for it among the committers.

I think that that's a reasonable thing to do, provided there is
obvious feedback that makes it highly unlikely that the committer will
make an error at the last moment. I have a hard time coming up with a
suggestion that won't be considered annoying by at least one person,
though.

Would it be awful if there was a #warning directive that kicked in
when the temporary OID range is in use? It should be possible to do
that without breaking -Werror builds, which I believe Robert uses (I
am reminded of the Flex bug that we used to have to work around). It's
not like there are that many patches that need to assign OIDs to new
catalog entries. I would suggest that we put the warning in the
regression tests if I didn't know that that could be missed by the use
of parallel variants, where the output flies by. There is no precedent
for using #warning for something like that, but offhand it seems like
the only thing that would work consistently.

I don't really mind having to do slightly more work when the issue
crops up, especially if that means less work for everyone involved in
aggregate, which is the cost that I'm concerned about the most.
However, an undocumented or under-documented process that requires a
fixed amount of extra mental effort when committing *anything* is
another matter.

-- 
Peter Geoghegan

  28. Re: Why don't we have a small reserved OID range for patch revisions?

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2019-02-28T23:57:04Z

    
On 3/1/19 12:41 AM, Peter Geoghegan wrote:
> On Thu, Feb 28, 2019 at 3:09 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The only thing that's really clear is that some senior committers don't
>> want to be bothered because they don't think there's a problem here that
>> justifies any additional expenditure of their time.  Perhaps they are
>> right, because I'd expected some comments from non-committer developers
>> confirming that they see a problem, and the silence is deafening.
> 
> I don't think that you can take that as too strong a signal. The
> incentives are different for non-committers.
> 
>> I'm inclined to commit some form of Naylor's tool improvement anyway,
>> because I have use for it; I remember times when I've renumbered OIDs
>> manually in patches, and it wasn't much fun.  But I can't force a
>> process change if there's not consensus for it among the committers.
> 
> I think that that's a reasonable thing to do, provided there is
> obvious feedback that makes it highly unlikely that the committer will
> make an error at the last moment. I have a hard time coming up with a
> suggestion that won't be considered annoying by at least one person,
> though.
> 

FWIW I personally would not mind if such tool / process was added. But I
have a related question - do we have some sort of list of such processes
that I could check? That is, a list of stuff that is expected to be done
by a committer before a commit?

I do recall we have [1], but perhaps we have something else.

https://wiki.postgresql.org/wiki/Committing_checklist


regards

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

  29. Re: Why don't we have a small reserved OID range for patch revisions?

    Peter Geoghegan <pg@bowt.ie> — 2019-02-28T23:57:12Z

    On Thu, Feb 28, 2019 at 3:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> >>> just as a thought, what if we stopped assigning manual OIDs for new
> >>> catalog entries altogether, except for once at the end of each release
> >>> cycle?
>
> Actually ... that leads to an idea that wouldn't add any per-commit
> overhead, or really much change at all to existing processes.  Given
> the existence of a reliable OID-renumbering tool, we could:

> In this scheme, OID collisions are a problem for in-progress patches
> only if two patches are unlucky enough to choose the same random
> high OIDs during the same devel cycle.  That's unlikely, or at least
> a good bit less likely than collisions are today.

That sounds like a reasonable compromise. Perhaps the unused_oids
script could give specific guidance on using a randomly determined
small range of contiguous OIDs that fall within the current range for
that devel cycle. That would prevent collisions caused by the natural
human tendency to prefer a round number. Having contiguous OIDs for
the same patch seems worth preserving.

-- 
Peter Geoghegan

  30. Re: Why don't we have a small reserved OID range for patch revisions?

    Robert Haas <robertmhaas@gmail.com> — 2019-03-01T19:36:50Z

    On Thu, Feb 28, 2019 at 6:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 1. Encourage people to develop new patches using chosen-at-random
> high OIDs, in the 7K-9K range.  They do this already, it'd just
> be encouraged instead of discouraged.
>
> 2. Commit patches as received.
>
> 3. Once each devel cycle, after feature freeze, somebody uses the
> renumbering tool to shove all the new OIDs down to lower numbers,
> freeing the high-OID range for the next devel cycle.  We'd have
> to remember to do that, but it could be added to the RELEASE_CHANGES
> checklist.

Sure, that sounds nice.  It seems like it might be slightly less
convenient for non-committers than what I was proposing, but still
more convenient than what they're doing right now.  And it's also more
convenient for committers, because they're not being asked to manually
fiddle patches at the last moment, something that I at least find
rather error-prone.  It also, and I think this is really good, moves
in the direction of fewer things for both patch authors and patch
committers to worry about doing wrong.  Instead of throwing rocks at
people whose OID assignments are "wrong," we just accept what people
do and adjust it later if it makes sense to do so.

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

  31. Re: Why don't we have a small reserved OID range for patch revisions?

    Robert Haas <robertmhaas@gmail.com> — 2019-03-01T19:56:10Z

    On Thu, Feb 28, 2019 at 5:36 PM Peter Geoghegan <pg@bowt.ie> wrote:
> I have attempted to institute some general guidelines for what the
> thicket of rules are by creating the "committing checklist" page. This
> is necessarily imperfect, because the rules are in many cases open to
> interpretation, often for good practical reasons. I don't have any
> sympathy for committers that find it hard to remember to do a
> catversion bump with any kind of regularity. That complexity seems
> inherent, not incidental, since it's often convenient to ignore
> catalog incompatibilities during development.

Bumping catversion is something of a special case, because one does it
so often that one gets used to remembering it.  The rules are usually
not too hard to remember, although they are trickier when you don't
directly change anything in src/include/catalog but just change the
definition of some node that may be serialized in a catalog someplace.
It would be neat if there were a tool you could run to somehow tell
you whether catversion needs to be changed for a given patch.

But you know there are a log of other version numbers floating around,
like XLOG_PAGE_MAGIC or the pg_dump archive version, and it is not
really that easy to know -- as a new contributor or sometimes even as
an experienced one -- whether your work requires any changes to that
stuff, or even that that stuff *exists*.  Indeed, XLOG_PAGE_MAGIC is a
particularly annoying case, both because the constant name doesn't
contain VERSION and because the comment just says /* can be used as
WAL version indicator */ which does not exactly make it clear that if
you fail to bump it when you touch the WAL format you will Make People
Unhappy.

Indeed, Simon got complaints a number of years ago (2010, it looks
like) when he had the temerity to change the magic number to some
other unrelated value instead of just incrementing it by one.
Although I think that the criticism was to a certain extent
well-founded -- why deviate from previous practice? -- there is at the
same time something a little crazy about somebody getting excited
about the particular value that has been chosen for a number that is
described in the very name of the constant as a MAGIC number.  And
especially because there is absolutely zip in the way of code comments
or a README that explain to you how to do it "right."

> > So, I suspect that for every
> > unit of work it saves somebody, it's probably going to generate about
> > one unit of extra work for somebody else.
>
> Maybe so. I think that you're jumping to conclusions, though.

I did say "I suspect..." which was intended as a concession that I
don't know for sure.

> But you seem to want to make the mechanism itself even more
> complicated, not less complicated (based on your remarks about making
> OID assignment happen during the build). In order to make the use of
> the mechanism easier. That seems worth considering, but ISTM that this
> is talking at cross purposes. There are far simpler ways of making it
> unlikely that a committer is going to miss this step. There is also a
> simple way of noticing that they do quickly (e.g. a simple buildfarm
> test).

Well, perhaps I'm proposing some additional code, but I don't think of
that as making the mechanism more complicated.  I want to make it
simpler for patch submitters and reviewers and committers to not make
mistakes that they have to run around and fix.  If there are fewer
kinds of things that qualify as mistakes, as in Tom's latest proposal,
then we are moving in the right direction IMO regardless of anything
else.

> > OK.  Well, I think that doing nothing is superior to this proposal,
> > for reasons similar to what Peter Eisentraut has already articulated.
> > And I think rather than blasting forward with your own preferred
> > alternative in the face of disagreement, you should be willing to
> > discuss other possible options.  But if you're not willing to do that,
> > I can't make you.
>
> Peter seemed to not want to do this on the grounds that it isn't
> necessary at all, whereas you think that it doesn't go far enough. If
> there is a consensus against what Tom has said, it's a cacophonous one
> that cannot really be said to be in favor of anything.

I think Peter and I are more agreeing than we are at the opposite ends
of a spectrum, but more importantly, I think it is worth having a
discussion first about what people like and dislike, and what goals
they have, and then only if necessary, counting the votes afterwards.
I don't like having the feeling that because I have a different view
of something and want to write an email about that, I am somehow an
impediment to progress.  I think if we reduce discussions to
you're-for-it-or-your-against-it, that's not that helpful.

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

  32. Re: Why don't we have a small reserved OID range for patch revisions?

    Peter Geoghegan <pg@bowt.ie> — 2019-03-01T20:42:02Z

    On Fri, Mar 1, 2019 at 11:56 AM Robert Haas <robertmhaas@gmail.com> wrote:
> It would be neat if there were a tool you could run to somehow tell
> you whether catversion needs to be changed for a given patch.

That seems infeasible because of stored rules. A lot of things bleed
into that. We could certainly do better at documenting this on the
"committing checklist" page, though.

> Indeed, Simon got complaints a number of years ago (2010, it looks
> like) when he had the temerity to change the magic number to some
> other unrelated value instead of just incrementing it by one.

Off with his head!

> Although I think that the criticism was to a certain extent
> well-founded -- why deviate from previous practice? -- there is at the
> same time something a little crazy about somebody getting excited
> about the particular value that has been chosen for a number that is
> described in the very name of the constant as a MAGIC number.  And
> especially because there is absolutely zip in the way of code comments
> or a README that explain to you how to do it "right."

I have learned to avoid ambiguity more than anything else, because
ambiguity causes patches to flounder indefinitely, whereas it's
usually not that hard to fix something that's broken. I agree -
anything that adds ambiguity rather than taking it away is a big
problem.

> Well, perhaps I'm proposing some additional code, but I don't think of
> that as making the mechanism more complicated.  I want to make it
> simpler for patch submitters and reviewers and committers to not make
> mistakes that they have to run around and fix.

Right. So do I. I just don't think that it's that bad to ask the final
committer to do something once, rather than getting everyone else
(including committers) to do it multiple times. If we can avoid even
this burden, and totally centralize the management of the OID space,
then so much the better.

> If there are fewer
> kinds of things that qualify as mistakes, as in Tom's latest proposal,
> then we are moving in the right direction IMO regardless of anything
> else.

I'm glad that we now have a plan that is a clear step forward.

> I think Peter and I are more agreeing than we are at the opposite ends
> of a spectrum, but more importantly, I think it is worth having a
> discussion first about what people like and dislike, and what goals
> they have, and then only if necessary, counting the votes afterwards.

I agree that that's totally worthwhile.

> I don't like having the feeling that because I have a different view
> of something and want to write an email about that, I am somehow an
> impediment to progress.  I think if we reduce discussions to
> you're-for-it-or-your-against-it, that's not that helpful.

That was not my intention. The way that you brought the issue of the
difficulty of being a contributor in general into it was unhelpful,
though. It didn't seem useful or fair to link Tom's position to a big,
well known controversy.

We now have a solution that everyone is happy with, or can at least
live with, which suggests to me that Tom wasn't being intransigent or
insensitive to the concerns of contributors.

-- 
Peter Geoghegan

  33. Re: Why don't we have a small reserved OID range for patch revisions?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-01T23:05:05Z

    Peter Geoghegan <pg@bowt.ie> writes:
> On Fri, Mar 1, 2019 at 11:56 AM Robert Haas <robertmhaas@gmail.com> wrote:
>> It would be neat if there were a tool you could run to somehow tell
>> you whether catversion needs to be changed for a given patch.

> That seems infeasible because of stored rules. A lot of things bleed
> into that. We could certainly do better at documenting this on the
> "committing checklist" page, though.

A first approximation to that is "did you touch readfuncs.c", though
that rule will give a false positive if you only changed Plan nodes.

			regards, tom lane

  34. Re: Why don't we have a small reserved OID range for patch revisions?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-08T17:14:09Z

    John Naylor <john.naylor@2ndquadrant.com> writes:
>> On 2/8/19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> A script such as you suggest might be a good way to reduce the temptation
>>> to get lazy at the last minute.  Now that the catalog data is pretty
>>> machine-readable, I suspect it wouldn't be very hard --- though I'm
>>> not volunteering either.  I'm envisioning something simple like "renumber
>>> all OIDs in range mmmm-nnnn into range xxxx-yyyy", perhaps with the
>>> ability to skip any already-used OIDs in the target range.

>> This might be something that can be done inside reformat_dat_files.pl.
>> It's a little outside it's scope, but better than the alternatives.

> Along those lines, here's a draft patch to do just that. It handles
> array type oids as well. Run it like this:

> perl  reformat_dat_file.pl  --map-from 9000  --map-to 2000  *.dat

I took a quick look at this.  I went ahead and pushed the parts that
were just code cleanup in reformat_dat_file.pl, since that seemed
pretty uncontroversial.  As far as the rest of it goes:

* I'm really not terribly happy with sticking this functionality into
reformat_dat_file.pl.  First, there's an issue of discoverability:
it's not obvious that a script named that way would have such an
ability.  Second, it clutters the script in a way that seems to me
to hinder its usefulness as a basis for one-off hacks.  So I'd really
rather have a separate script named something like "renumber_oids.pl",
even if there's a good deal of code duplication between it and
reformat_dat_file.pl.

* In my vision of what this might be good for, I think it's important
that it be possible to specify a range of input OIDs to renumber, not
just "everything above N".  I agree the output range only needs a
starting OID.

BTW, I changed the CF entry's target back to v12; I don't see a
reason not to get this done this month, and indeed kind of wish
it was available right now ;-)

			regards, tom lane

  35. Re: Why don't we have a small reserved OID range for patch revisions?

    John Naylor <john.naylor@2ndquadrant.com> — 2019-03-10T17:11:18Z

    On Sat, Mar 9, 2019 at 1:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I took a quick look at this.  I went ahead and pushed the parts that
> were just code cleanup in reformat_dat_file.pl, since that seemed
> pretty uncontroversial.  As far as the rest of it goes:

Okay, thanks.

> * I'm really not terribly happy with sticking this functionality into
> reformat_dat_file.pl.  First, there's an issue of discoverability:
> it's not obvious that a script named that way would have such an
> ability.  Second, it clutters the script in a way that seems to me
> to hinder its usefulness as a basis for one-off hacks.  So I'd really
> rather have a separate script named something like "renumber_oids.pl",
> even if there's a good deal of code duplication between it and
> reformat_dat_file.pl.

> * In my vision of what this might be good for, I think it's important
> that it be possible to specify a range of input OIDs to renumber, not
> just "everything above N".  I agree the output range only needs a
> starting OID.

Now it looks like:

perl  renumber_oids.pl  --first-mapped-oid 8000 --last-mapped-oid 8999
 --first-target-oid 2000  *.dat

To prevent a maintenance headache, I didn't copy any of the formatting
logic over. You'll also have to run reformat_dat_files.pl afterwards
to restore that. It seems to work, but I haven't tested thoroughly.

-- 
John Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  36. Re: Why don't we have a small reserved OID range for patch revisions?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-11T21:36:24Z

    John Naylor <john.naylor@2ndquadrant.com> writes:
> Now it looks like:
> perl  renumber_oids.pl  --first-mapped-oid 8000 --last-mapped-oid 8999
>  --first-target-oid 2000  *.dat
> To prevent a maintenance headache, I didn't copy any of the formatting
> logic over. You'll also have to run reformat_dat_files.pl afterwards
> to restore that. It seems to work, but I haven't tested thoroughly.

I didn't like the use of Data::Dumper, because it made it quite impossible
to check what the script had done by eyeball.  After some thought
I concluded that we could probably just apply the changes via
search-and-replace, which is pretty ugly and low-tech but it leads to
easily diffable results, whether or not the initial state is exactly
what reformat_dat_files would produce.

I also changed things so that the OID mapping is computed before we start
changing any files, because as it stood the objects would get renumbered
in a pretty random order; and I renamed one of the switches so they all
have unique one-letter abbreviations.

Experimenting with this, I realized that it couldn't renumber OIDs that
are defined in .h files rather than .dat files, which is a serious
deficiency, but given the search-and-replace implementation it's not too
hard to fix up the .h files as well.  So I did that, and removed the
expectation that the target files would be listed on the command line;
that seems more likely to be a foot-gun than to do anything useful.

I've successfully done check-world after renumbering every OID above
4000 to somewhere else.  I also tried renumbering everything below
4000, which unsurprisingly blew up because there are various catalog
columns we haven't fixed to use symbolic OIDs.  (The one that initdb
immediately trips over is pg_database.dattablespace.)  I'm not sure
if it's worth the trouble to make that totally clean, but I suspect
we ought to at least mop up text-search references to be symbolic.
That's material for a separate patch though.

This seems committable from my end --- any further comments?

			regards, tom lane

  37. Re: Why don't we have a small reserved OID range for patch revisions?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-12T04:56:18Z

    I wrote:
> I've successfully done check-world after renumbering every OID above
> 4000 to somewhere else.  I also tried renumbering everything below
> 4000, which unsurprisingly blew up because there are various catalog
> columns we haven't fixed to use symbolic OIDs.  (The one that initdb
> immediately trips over is pg_database.dattablespace.)  I'm not sure
> if it's worth the trouble to make that totally clean, but I suspect
> we ought to at least mop up text-search references to be symbolic.
> That's material for a separate patch though.

So I couldn't resist poking at that, and after a couple hours' work
I have the attached patch, which removes all remaining hard-wired
OID references in the .dat files.

Using this, I renumbered all the OIDs in include/catalog, and behold
things pretty much worked.  I got through check-world after hacking
up these points:

* Unsurprisingly, there are lots of regression tests that have object
OIDs hard-wired in queries and/or expected output.

* initdb.c has a couple of places that know that template1 has OID 1.

* information_schema.sql has several SQL-language functions that
hard-wire the OIDs of assorted built-in types.

I'm not particularly fussed about the first two points, but the
last is a bit worrisome.  It's not too hard to imagine somebody
adding knowledge of their new type to those functions, and the
code getting broken by a renumbering pass, and us not noticing
if the point isn't stressed by a regression test (which mostly
those functions aren't).

We could imagine fixing those functions along the lines of

   CASE WHEN $2 = -1 /* default typmod */
        THEN null
-       WHEN $1 IN (1042, 1043) /* char, varchar */
+       WHEN $1 IN ('pg_catalog.bpchar'::pg_catalog.regtype,
+                   'pg_catalog.varchar'::pg_catalog.regtype)
        THEN $2 - 4

which would add some parsing overhead, but I'm not sure if anyone
would notice that.

			regards, tom lane

  38. Re: Why don't we have a small reserved OID range for patch revisions?

    John Naylor <john.naylor@2ndquadrant.com> — 2019-03-12T07:22:24Z

    On Tue, Mar 12, 2019 at 5:36 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> This seems committable from my end --- any further comments?

I gave it a read and it looks good to me, but I haven't tried to run it.

-- 
John Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  39. Re: Why don't we have a small reserved OID range for patch revisions?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-12T16:50:19Z

    John Naylor <john.naylor@2ndquadrant.com> writes:
> On Tue, Mar 12, 2019 at 5:36 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This seems committable from my end --- any further comments?

> I gave it a read and it looks good to me, but I haven't tried to run it.

Thanks for checking.  I've pushed both patches now.

I noticed while looking at the pg_class data that someone had stuck in a
hack to make genbki.pl substitute for "PGHEAPAM", which AFAICS is just
following the bad old precedent of PGNSP and PGUID.  I got rid of that
in favor of using the already-existing BKI_LOOKUP(pg_am) mechanism.
Maybe someday we should try to get rid of PGNSP and PGUID too, although
there are stumbling blocks in the way of both:

* PGNSP is also substituted for in the bodies of some SQL procedures.

* Replacing PGUID with the actual name of the bootstrap superuser is a
bit problematic because that name isn't necessarily "postgres".  We
could probably make it work, but I'm not convinced it'd be any less
confusing than the existing special-case behavior is.

Anyway I think we're basically done here.  There's some additional
cleanup that could possibly be done, like removing the hard-wired
references to OID 1 in initdb.c.  But I'm having a hard time convincing
myself that it's worth the trouble, except maybe for the question of
information_schema.sql's hard-wired type OIDs.  Even there, it's
certainly possible for a patch to use a regtype constant even if
the existing code doesn't.

			regards, tom lane