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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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