The unused_oids script should have a reminder to use the 8000-8999 OID range

On Thu, Aug 1, 2019 at 10:37 PM Peter Geoghegan <pg@bowt.ie> wrote:

I pushed a commit that required a new pg_proc entry today. Had I not
been involved with the work that became commit a6417078, I would
definitely not have used an OID from the range reserved for devel
system catalogs (8000 - 8999). As I understand it, this is now
standard practice.

Perhaps unsurprisingly, other committers didn't get the memo, and
haven't been using the special reserved range since its introduction
in March. I think that this could be avoided by simply making
unused_oids print a reminder about the new practice.

Huge +1. Last time I had to pick a new oid it took me ages to find
the correct range for that. The script could even suggest a random
free oid in the range, for extra laziness as you also suggested in the
almost exact same mail at
CAH2-WzmCzNMebiN4-8p=ON92m0Rz0ybxNEKrO_2J+9DqWfWP=A@mail.gmail.com :)

On Thu, Aug 1, 2019 at 1:57 PM Julien Rouhaud <rjuju123@gmail.com> wrote:

Huge +1. Last time I had to pick a new oid it took me ages to find
the correct range for that. The script could even suggest a random
free oid in the range, for extra laziness as you also suggested in the
almost exact same mail at
CAH2-WzmCzNMebiN4-8p=ON92m0Rz0ybxNEKrO_2J+9DqWfWP=A@mail.gmail.com :)

Seems like I should propose a patch this time around. I don't do Perl,
but I suppose I could manage something as trivial as this.

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:

Is it within the discretion of committers to not use the reserved
range? It seems preferable for everybody to consistently use the
reserved OID range.

I think it's up to the committer in the end. But if someone submits
a patch using high OIDs, I for one would not change that (unless it
had a collision through bad luck).

I agree that adjusting the unused_oids script would be an appropriate
thing to do now.

regards, tom lane

On Thu, Aug 01, 2019 at 06:59:06PM -0700, Peter Geoghegan wrote:

Seems like I should propose a patch this time around. I don't do Perl,
but I suppose I could manage something as trivial as this.

Well, that new project policy is not that well-advertised then, see
for example the recent 5925e55, c085e1c and 313f87a. So having some
kind of safety net would be nice.
--
Michael

On Fri, Aug 2, 2019 at 6:21 AM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Aug 01, 2019 at 06:59:06PM -0700, Peter Geoghegan wrote:

Seems like I should propose a patch this time around. I don't do Perl,
but I suppose I could manage something as trivial as this.

Well, that new project policy is not that well-advertised then, see
for example the recent 5925e55, c085e1c and 313f87a. So having some
kind of safety net would be nice.

Trivial patch for that attached. The output is now like:

[...]
Using an oid in the 8000-9999 range is recommended.
For instance: 9427

(checking that the suggested random oid is not used yet.)

On Fri, Aug 2, 2019 at 1:42 AM Julien Rouhaud <rjuju123@gmail.com> wrote:

Trivial patch for that attached.

Thanks!

The output is now like:

[...]
Using an oid in the 8000-9999 range is recommended.
For instance: 9427

(checking that the suggested random oid is not used yet.)

I've taken your patch, and changed the wording a bit. I think that
it's worth being a bit more explicit. The attached revision produces
output that looks like this:

Patches should use a more-or-less consecutive range of OIDs.
Best practice is to make a random choice in the range 8000-9999.
Suggested random unused OID: 9099

I would like to push this patch shortly. How do people feel about this
wording? (It's based on the documentation added by commit a6417078.)

--
Peter Geoghegan

Le ven. 2 août 2019 à 20:12, Peter Geoghegan <pg@bowt.ie> a écrit :

On Fri, Aug 2, 2019 at 1:42 AM Julien Rouhaud <rjuju123@gmail.com> wrote:

Trivial patch for that attached.

Thanks!

The output is now like:

[...]
Using an oid in the 8000-9999 range is recommended.
For instance: 9427

(checking that the suggested random oid is not used yet.)

I've taken your patch, and changed the wording a bit. I think that
it's worth being a bit more explicit. The attached revision produces
output that looks like this:

Patches should use a more-or-less consecutive range of OIDs.
Best practice is to make a random choice in the range 8000-9999.
Suggested random unused OID: 9099

I would like to push this patch shortly. How do people feel about this
wording? (It's based on the documentation added by commit a6417078.)

I'm fine with it!

Peter Geoghegan <pg@bowt.ie> writes:

I've taken your patch, and changed the wording a bit. I think that
it's worth being a bit more explicit. The attached revision produces
output that looks like this:

Patches should use a more-or-less consecutive range of OIDs.
Best practice is to make a random choice in the range 8000-9999.
Suggested random unused OID: 9099

Maybe s/make a/start with/ ?

Also, once people start doing this, it'd be unfriendly to suggest
9099 if 9100 is already committed. There should be some attention
to *how many* consecutive free OIDs will be available if one starts
at the suggestion. You could perhaps print "9099 (42 OIDs available
starting here)", and if the user doesn't like the amount of headroom
in that, they could just run it again for a different suggestion.

regards, tom lane

On Fri, Aug 2, 2019 at 1:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Maybe s/make a/start with/ ?

Also, once people start doing this, it'd be unfriendly to suggest
9099 if 9100 is already committed. There should be some attention
to *how many* consecutive free OIDs will be available if one starts
at the suggestion.

How about this wording?:

Patches should use a more-or-less consecutive range of OIDs.
Best practice is to start with a random choice in the range 8000-9999.
Suggested random unused OID: 9591 (409 consecutive OID(s) available
starting here)

Attached is v3, which implements your suggestion, generating output
like the above. I haven't written a line of Perl in my life prior to
today, so basic code review would be helpful.

--
Peter Geoghegan

On Fri, 2 Aug 2019 at 16:49, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Geoghegan <pg@bowt.ie> writes:

I've taken your patch, and changed the wording a bit. I think that
it's worth being a bit more explicit. The attached revision produces
output that looks like this:

Patches should use a more-or-less consecutive range of OIDs.
Best practice is to make a random choice in the range 8000-9999.
Suggested random unused OID: 9099

Noob question here: why not start with the next unused OID in the range,
and on the other hand reserve the range for sequentially-assigned values?

On Fri, Aug 2, 2019 at 2:52 PM Isaac Morland <isaac.morland@gmail.com> wrote:

Noob question here: why not start with the next unused OID in the range, and on the other hand reserve the range for sequentially-assigned values?

The general idea is to avoid OID collisions while a patch is under
development. Choosing a value that aligns nicely with
already-allocated OIDs makes these collisions much more likely, which
commit a6417078 addressed back in March. We want a random choice among
patches, but OIDs used within a patch should be consecutive.

(There is still some chance of a collision, but you have to be fairly
unlucky to have that happen under the system introduced by commit
a6417078.)

It's probably the case that most patches that create a new pg_proc
entry only create one. The question of consecutive OIDs only comes up
with a fairly small number of patches.

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:

Attached is v3, which implements your suggestion, generating output
like the above. I haven't written a line of Perl in my life prior to
today, so basic code review would be helpful.

The "if ($oid > $prev_oid + 2)" test seems unnecessary.
It's certainly wrong to keep iterating beyond the first
oid that's > $suggestion.

Perhaps you meant to go back and try a different suggestion
if there's not at least 2 free OIDs? But then there needs
to be an outer loop around both of these loops.

regards, tom lane

On Fri, Aug 2, 2019 at 3:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

The "if ($oid > $prev_oid + 2)" test seems unnecessary.
It's certainly wrong to keep iterating beyond the first
oid that's > $suggestion.

Sorry. That was just carelessness on my part. (Being the world's worst
Perl programmer is no excuse.)

How about the attached? I've simply removed the "if ($oid > $prev_oid
+ 2)" test.

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:

How about the attached? I've simply removed the "if ($oid > $prev_oid
+ 2)" test.

Better ... but I'm the world's second worst Perl programmer,
so I have little to say about whether it's idiomatic.

regards, tom lane

On Fri, Aug 2, 2019 at 3:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Better ... but I'm the world's second worst Perl programmer,
so I have little to say about whether it's idiomatic.

Perhaps Michael can weigh in here? I'd rather hear a second opinion on
v4 of the patch before proceeding.

--
Peter Geoghegan

On Sat, Aug 3, 2019 at 2:40 AM Peter Geoghegan <pg@bowt.ie> wrote:

On Fri, Aug 2, 2019 at 3:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Better ... but I'm the world's second worst Perl programmer,
so I have little to say about whether it's idiomatic.

Perhaps Michael can weigh in here? I'd rather hear a second opinion on
v4 of the patch before proceeding.

I probably write less perl than Michael, but it looks just fine to me.

On Sat, Aug 03, 2019 at 11:40:24AM +0200, Julien Rouhaud wrote:

I probably write less perl than Michael, but it looks just fine to me.

Indentation with pgperltidy complains with the attached diff (based on
top of v4).

+printf "Patches should use a more-or-less consecutive range of OIDs.\n";
"Patches should try to use a consecutive range of OIDs"?

Why choosing a random position within [8000,9999]? This leads to the
following messages for example with multiple runs, which is confusing:
Suggested random unused OID: 9473 (527 consecutive OID(s) available
Suggested random unused OID: 8159 (31 consecutive OID(s) available
Suggested random unused OID: 9491 (509 consecutive OID(s) available

Wouldn't it be better to choose the lowest position in the development
range, and then adapt the suggestion based on that? We could
recommend the range if there are at least 10 OIDs available in the
range from the lowest position, and there are few patches eating more
than 5-10 OIDs at once.
--
Michael

On Sat, Aug 3, 2019 at 7:48 PM Michael Paquier <michael@paquier.xyz> wrote:

Why choosing a random position within [8000,9999]? This leads to the
following messages for example with multiple runs, which is confusing:
Suggested random unused OID: 9473 (527 consecutive OID(s) available
Suggested random unused OID: 8159 (31 consecutive OID(s) available
Suggested random unused OID: 9491 (509 consecutive OID(s) available

Wouldn't it be better to choose the lowest position in the development
range, and then adapt the suggestion based on that?

No, it wouldn't. The entire point of suggesting a totally random OID
is that it minimizes the probability of a collision among concurrently
developed patches, per the policy established by commit a6417078 --
what you suggest would defeat the very purpose of this patch. In fact,
having everybody see the same suggestion from unused_oids would
*maximize* the number of OID collisions.

We could
recommend the range if there are at least 10 OIDs available in the
range from the lowest position, and there are few patches eating more
than 5-10 OIDs at once.

That sounds like an over-engineered solution to a problem that doesn't exist.

--
Peter Geoghegan

On Fri, Aug 2, 2019 at 1:28 PM Julien Rouhaud <rjuju123@gmail.com> wrote:

I'm fine with it!

Pushed a version with similar wording just now.

Thanks!
--
Peter Geoghegan