Re: Adding a pg_get_owned_sequence function?

Hi hackers,

I've always been annoyed by the fact that pg_get_serial_sequence takes
the table and returns the sequence as strings rather than regclass. And
since identity columns were added, the name is misleading as well (which
is even acknowledged in the docs, together with a suggestion for a
better name).

So, instead of making excuses in the documentation, I thought why not
add a new function which addresses all of these issues, and document the
old one as a backward-compatibilty wrapper?

Please see the attached patch for my stab at this.

- ilmari

On Fri, 9 Jun 2023, at 20:19, Dagfinn Ilmari Mannsåker wrote:

> Hi hackers,
>
> I've always been annoyed by the fact that pg_get_serial_sequence takes
> the table and returns the sequence as strings rather than regclass. And
> since identity columns were added, the name is misleading as well (which
> is even acknowledged in the docs, together with a suggestion for a
> better name).
>
> So, instead of making excuses in the documentation, I thought why not
> add a new function which addresses all of these issues, and document the
> old one as a backward-compatibilty wrapper?
>
> Please see the attached patch for my stab at this.

This didn't get any replies, so I've created a commitfest entry to make sure it doesn't get lost:

https://commitfest.postgresql.org/44/4535/

--
- ilmari

On Fri, Jun 09, 2023 at 08:19:44PM +0100, Dagfinn Ilmari Mannsåker wrote:
> I've always been annoyed by the fact that pg_get_serial_sequence takes
> the table and returns the sequence as strings rather than regclass. And
> since identity columns were added, the name is misleading as well (which
> is even acknowledged in the docs, together with a suggestion for a
> better name).
>
> So, instead of making excuses in the documentation, I thought why not
> add a new function which addresses all of these issues, and document the
> old one as a backward-compatibilty wrapper?

This sounds generally reasonable to me. That note has been there since
2006 (2b2a507). I didn't find any further discussion about this on the
lists.

> + A backwards-compatibility wrapper
> + for <function>pg_get_owned_sequence</function>, which
> + uses <type>text</type> for the table and sequence names instead of
> + <type>regclass</type>. The first parameter is a table name with optional

I wonder if it'd be possible to just remove pg_get_serial_sequence().
Assuming 2b2a507 removed the last use of it in pg_dump, any dump files
created on versions >= v8.2 shouldn't use it. But I suppose it wouldn't be
too much trouble to keep it around for anyone who happens to need it.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Nathan Bossart <nathandbossart(at)gmail(dot)com> writes:
> I wonder if it'd be possible to just remove pg_get_serial_sequence().

A quick search at http://codesearch.debian.net/ finds uses of it
in packages like gdal, qgis, rails, ... We could maybe get rid of
it after a suitable deprecation period, but I think we can't just
summarily remove it.

regards, tom lane

On Fri, Sep 01, 2023 at 01:31:43PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart(at)gmail(dot)com> writes:
>> I wonder if it'd be possible to just remove pg_get_serial_sequence().
>
> A quick search at http://codesearch.debian.net/ finds uses of it
> in packages like gdal, qgis, rails, ... We could maybe get rid of
> it after a suitable deprecation period, but I think we can't just
> summarily remove it.

Given that, I'd still vote for marking it deprecated, but I don't feel
strongly about actually removing it anytime soon (or anytime at all,
really).

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Greetings,

* Nathan Bossart (nathandbossart(at)gmail(dot)com) wrote:
> On Fri, Sep 01, 2023 at 01:31:43PM -0400, Tom Lane wrote:
> > Nathan Bossart <nathandbossart(at)gmail(dot)com> writes:
> >> I wonder if it'd be possible to just remove pg_get_serial_sequence().
> >
> > A quick search at http://codesearch.debian.net/ finds uses of it
> > in packages like gdal, qgis, rails, ... We could maybe get rid of
> > it after a suitable deprecation period, but I think we can't just
> > summarily remove it.

I don't agree with this- we would only be removing it from the next
major release which is a year away and our other major releases will be
supported for years to come. If we do remove it, it'd be great to
mention it to those projects and ask them to update ahead of the next
release.

> Given that, I'd still vote for marking it deprecated, but I don't feel
> strongly about actually removing it anytime soon (or anytime at all,
> really).

Why would we mark it as deprecated then? If we're not going to get rid
of it, then we're supporting it and we'll fix issues with it and that
basically means it's not deprecated. If it's broken and we're not going
to fix it, then we should get rid of it.

If we're going to actually mark it deprecated then it should be, at
least, a yearly discussion about removing it. I'm generally more in
favor of either just keeping it, or just removing it, though. We've had
very little success marking things as deprecated as a way of getting
everyone to stop using it- some folks will stop using it right away and
those are the same people who would just adapt to it being gone in the
next major version quickly, and then there's folks who won't do anything
until it's actually gone (and maybe not even then). There really isn't
a serious middle-ground where deprecation is helpful given our yearly
release cycle and long major version support period.

Thanks,

Stephen

On Fri, Sep 08, 2023 at 10:56:15AM -0400, Stephen Frost wrote:
> If we're going to actually mark it deprecated then it should be, at
> least, a yearly discussion about removing it. I'm generally more in
> favor of either just keeping it, or just removing it, though. We've had
> very little success marking things as deprecated as a way of getting
> everyone to stop using it- some folks will stop using it right away and
> those are the same people who would just adapt to it being gone in the
> next major version quickly, and then there's folks who won't do anything
> until it's actually gone (and maybe not even then). There really isn't
> a serious middle-ground where deprecation is helpful given our yearly
> release cycle and long major version support period.

Fair point.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 09.06.23 21:19, Dagfinn Ilmari Mannsåker wrote:
> I've always been annoyed by the fact that pg_get_serial_sequence takes
> the table and returns the sequence as strings rather than regclass. And
> since identity columns were added, the name is misleading as well (which
> is even acknowledged in the docs, together with a suggestion for a
> better name).

If you are striving for less misleading terminology, note that the
concept of an "owned sequence" does not exist for users of identity
sequences, and ALTER SEQUENCE / OWNED BY cannot be used for such sequences.

Would it work to just overload pg_get_serial_sequence with regclass
argument types?

Peter Eisentraut <peter(at)eisentraut(dot)org> writes:
> Would it work to just overload pg_get_serial_sequence with regclass
> argument types?

Probably not; the parser would have no principled way to resolve
pg_get_serial_sequence('foo', 'bar') as one or the other. I'm
not sure offhand if it would throw error or just choose one, but
if it just chooses one it'd likely be the text variant.

It's possible that we could get away with just summarily changing
the argument type from text to regclass. ISTR that we did exactly
that with nextval() years ago, and didn't get too much push-back.
But we couldn't do the same for the return type. Also, this
approach does nothing for the concern about the name being
misleading.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Peter Eisentraut <peter(at)eisentraut(dot)org> writes:
>> Would it work to just overload pg_get_serial_sequence with regclass
>> argument types?
>
> Probably not; the parser would have no principled way to resolve
> pg_get_serial_sequence('foo', 'bar') as one or the other. I'm
> not sure offhand if it would throw error or just choose one, but
> if it just chooses one it'd likely be the text variant.

That works fine, and it prefers the text version:

~=# create function pg_get_serial_sequence(tbl regclass, col name)
returns regclass strict stable parallel safe
return pg_get_serial_sequence(tbl::text, col::text)::regclass;
CREATE FUNCTION

~=# select pg_typeof(pg_get_serial_sequence('identest', 'id'));
┌───────────┐
│ pg_typeof │
├───────────┤
│ text │
└───────────┘
(1 row)

~=# select pg_typeof(pg_get_serial_sequence('identest', 'id'::name));
┌───────────┐
│ pg_typeof │
├───────────┤
│ regclass │
└───────────┘
(1 row)

> It's possible that we could get away with just summarily changing
> the argument type from text to regclass. ISTR that we did exactly
> that with nextval() years ago, and didn't get too much push-back.
> But we couldn't do the same for the return type. Also, this
> approach does nothing for the concern about the name being
> misleading.

Maybe we should go all the way the other way, and call it
pg_get_identity_sequence() and claim that "serial" is a legacy form of
identity columns?

- ilmari

On Tue, Sep 12, 2023 at 03:53:28PM +0100, Dagfinn Ilmari Mannsåker wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> It's possible that we could get away with just summarily changing
>> the argument type from text to regclass. ISTR that we did exactly
>> that with nextval() years ago, and didn't get too much push-back.
>> But we couldn't do the same for the return type. Also, this
>> approach does nothing for the concern about the name being
>> misleading.
>
> Maybe we should go all the way the other way, and call it
> pg_get_identity_sequence() and claim that "serial" is a legacy form of
> identity columns?

Hm. Could we split it into two functions, pg_get_owned_sequence() and
pg_get_identity_sequence()? I see that commit 3012061 [0] added support
for identity columns to pg_get_serial_sequence() because folks expected
that to work, so maybe that's a good reason to keep them together. If we
do elect to keep them combined, I'd be okay with renaming it to
pg_get_identity_sequence() along with your other proposed changes.

[0] https://postgr.es/m/20170912212054.25640.55202%40wrigleys.postgresql.org

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, Dec 8, 2023 at 3:43 PM Dagfinn Ilmari Mannsåker
<ilmari(at)ilmari(dot)org> wrote:
>
> Hi hackers,
>
> I've always been annoyed by the fact that pg_get_serial_sequence takes
> the table and returns the sequence as strings rather than regclass. And
> since identity columns were added, the name is misleading as well (which
> is even acknowledged in the docs, together with a suggestion for a
> better name).
>
> So, instead of making excuses in the documentation, I thought why not
> add a new function which addresses all of these issues, and document the
> old one as a backward-compatibilty wrapper?
>
> Please see the attached patch for my stab at this.
>

I reviewed the Patch and the compilation looks fine. I tested various
scenarios and did not find any issues. Also I did RUN 'make check'
and 'make check-world' and all the test cases passed successfully. I
figured out a small typo please have a look at it:-

This is the name the docs say `pg_get_serial_sequence` sholud have
had, and gives us the opportunity to change the return and table
argument types to `regclass` and the column argument to `name`,
instead of using `text` everywhere. This matches what's in catalogs,
and requires less explaining than the rules for
`pg_get_serial_sequence`.

Here 'sholud' have been 'should'.

Thanks and Regards,
Shubham Khanna.

On Tue, 24 Oct 2023 at 22:00, Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>
> On Tue, Sep 12, 2023 at 03:53:28PM +0100, Dagfinn Ilmari Mannsåker wrote:
> > Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> >> It's possible that we could get away with just summarily changing
> >> the argument type from text to regclass. ISTR that we did exactly
> >> that with nextval() years ago, and didn't get too much push-back.
> >> But we couldn't do the same for the return type. Also, this
> >> approach does nothing for the concern about the name being
> >> misleading.
> >
> > Maybe we should go all the way the other way, and call it
> > pg_get_identity_sequence() and claim that "serial" is a legacy form of
> > identity columns?
>
> Hm. Could we split it into two functions, pg_get_owned_sequence() and
> pg_get_identity_sequence()? I see that commit 3012061 [0] added support
> for identity columns to pg_get_serial_sequence() because folks expected
> that to work, so maybe that's a good reason to keep them together. If we
> do elect to keep them combined, I'd be okay with renaming it to
> pg_get_identity_sequence() along with your other proposed changes.

I have changed the status of the patch to "Waiting on Author" as
Nathan's comments have not yet been followed up.

Regards,
Vignesh

vignesh C <vignesh21(at)gmail(dot)com> writes:

> On Tue, 24 Oct 2023 at 22:00, Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>>
>> On Tue, Sep 12, 2023 at 03:53:28PM +0100, Dagfinn Ilmari Mannsåker wrote:
>> > Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> >> It's possible that we could get away with just summarily changing
>> >> the argument type from text to regclass. ISTR that we did exactly
>> >> that with nextval() years ago, and didn't get too much push-back.
>> >> But we couldn't do the same for the return type. Also, this
>> >> approach does nothing for the concern about the name being
>> >> misleading.
>> >
>> > Maybe we should go all the way the other way, and call it
>> > pg_get_identity_sequence() and claim that "serial" is a legacy form of
>> > identity columns?
>>
>> Hm. Could we split it into two functions, pg_get_owned_sequence() and
>> pg_get_identity_sequence()? I see that commit 3012061 [0] added support
>> for identity columns to pg_get_serial_sequence() because folks expected
>> that to work, so maybe that's a good reason to keep them together. If we
>> do elect to keep them combined, I'd be okay with renaming it to
>> pg_get_identity_sequence() along with your other proposed changes.

We can't make pg_get_serial_sequence(text, text) not work on identity
columns any more, that would break existing users, and making the new
function not work on serial columns would make it harder for people to
migrate to it. If you're suggesting adding two new functions,
pg_get_identity_sequence(regclass, name) and
pg_get_serial_sequence(regclass, name)¹, which only work on the type of
column corresponding to the name, I don't think that's great for
usability or migratability either.

> I have changed the status of the patch to "Waiting on Author" as
> Nathan's comments have not yet been followed up.

Thanks for the nudge, here's an updated patch that together with the
above addresses them. I've changed the commitfest entry back to "Needs
review".

> Regards,
> Vignesh

- ilmari

On Mon, Jan 08, 2024 at 04:58:02PM +0000, Dagfinn Ilmari Mannsåker wrote:
> We can't make pg_get_serial_sequence(text, text) not work on identity
> columns any more, that would break existing users, and making the new
> function not work on serial columns would make it harder for people to
> migrate to it. If you're suggesting adding two new functions,
> pg_get_identity_sequence(regclass, name) and
> pg_get_serial_sequence(regclass, name)¹, which only work on the type of
> column corresponding to the name, I don't think that's great for
> usability or migratability either.

I think these are reasonable concerns, but with this patch, we now have the
following functions:

pg_get_identity_sequence(table regclass, column name) -> regclass
pg_get_serial_sequence(table text, column text) -> text

If we only look at the names, it sure sounds like the first one only works
for identity columns, and the second only works for serial columns. But
both work for identity _and_ serial. The real differences between the two
are the parameter and return types. Granted, this is described in the
documentation updates, but IMHO this is a kind-of bizarre state to end up
in.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 08.01.24 22:08, Nathan Bossart wrote:
> I think these are reasonable concerns, but with this patch, we now have the
> following functions:
>
> pg_get_identity_sequence(table regclass, column name) -> regclass
> pg_get_serial_sequence(table text, column text) -> text
>
> If we only look at the names, it sure sounds like the first one only works
> for identity columns, and the second only works for serial columns. But
> both work for identity_and_ serial. The real differences between the two
> are the parameter and return types. Granted, this is described in the
> documentation updates, but IMHO this is a kind-of bizarre state to end up
> in.

Yeah, that's really weird.

Would it work to change the signature of pg_get_serial_sequence to

pg_get_serial_sequence(table anyelement, column text) -> anyelement

and then check inside the function code whether text or regclass was passed?

Peter Eisentraut <peter(at)eisentraut(dot)org> writes:
> Would it work to change the signature of pg_get_serial_sequence to
> pg_get_serial_sequence(table anyelement, column text) -> anyelement
> and then check inside the function code whether text or regclass was passed?

Probably not very well, because then we'd get no automatic coercion of
inputs that were not either type.

Maybe it would work to have both

pg_get_serial_sequence(table text, column text) -> text
pg_get_serial_sequence(table regclass, column text) -> regclass

but I wonder if that would create any situations where the parser
couldn't choose between these candidates.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Peter Eisentraut <peter(at)eisentraut(dot)org> writes:
>> Would it work to change the signature of pg_get_serial_sequence to
>> pg_get_serial_sequence(table anyelement, column text) -> anyelement
>> and then check inside the function code whether text or regclass was passed?
>
> Probably not very well, because then we'd get no automatic coercion of
> inputs that were not either type.
>
> Maybe it would work to have both
>
> pg_get_serial_sequence(table text, column text) -> text
> pg_get_serial_sequence(table regclass, column text) -> regclass

I think it would be more correct to use name for the column argument
type, rather than text.

> but I wonder if that would create any situations where the parser
> couldn't choose between these candidates.

According to my my earlier testing¹, the parser prefers the text version
for unknown-type arguments, and further testing shows that that's also
the case for other types with implicit casts to text such as varchar and
name. The regclass version gets chosen for oid and (big)int, because of
the implicit cast from (big)int to oid and oid to regclass.

The only case I could foresee failing would be types that have implicit
casts to both text and regtype. It turns out that varchar does have
both, but the parser still picks the text version without copmlaint.
Does it prefer the binary-coercible cast over the one that requires
calling a conversion function?

> regards, tom lane

- ilmari

[1]: https://postgr.es/m/87v8cfo32v.fsf@wibble.ilmari.org

=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari(at)ilmari(dot)org> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> Maybe it would work to have both
>> pg_get_serial_sequence(table text, column text) -> text
>> pg_get_serial_sequence(table regclass, column text) -> regclass

> I think it would be more correct to use name for the column argument
> type, rather than text.

In a green field perhaps, but we're not in a green field:
pg_get_serial_sequence(text, text)
already exists. That being the case, I'd strongly recommend that if
we overload this function name then we stick to text for the column
argument. If you add
pg_get_serial_sequence(regclass, name)
then you will be presenting the parser with situations where one
alternative is "more desirable" for one argument and "less desirable"
for the other, so that it's unclear which it will choose or whether
it will throw up its hands and refuse to choose.

> The only case I could foresee failing would be types that have implicit
> casts to both text and regtype. It turns out that varchar does have
> both, but the parser still picks the text version without copmlaint.
> Does it prefer the binary-coercible cast over the one that requires
> calling a conversion function?

Without having checked the code, I don't recall that there's any
preference for binary-coercible casts. But there definitely is
a preference for casting to "preferred" types, which text is
and these other types are not. That's why I'm afraid of your
use-name-not-text proposal: it puts the regclass alternative at an
even greater disadvantage in terms of the cast-choice heuristics.

regards, tom lane