Обсуждение: 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

From 7fd37c15920b7d2e87edef4351932559e2c9ef4f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Fri, 9 Jun 2023 19:55:58 +0100
Subject: [PATCH] Add pg_get_owned_sequence function

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`.
---
 doc/src/sgml/func.sgml                 | 46 ++++++++++++-----
 src/backend/utils/adt/ruleutils.c      | 69 +++++++++++++++++++-------
 src/include/catalog/pg_proc.dat        |  3 ++
 src/test/regress/expected/identity.out |  6 +++
 src/test/regress/expected/sequence.out |  6 +++
 src/test/regress/sql/identity.sql      |  1 +
 src/test/regress/sql/sequence.sql      |  1 +
 7 files changed, 102 insertions(+), 30 deletions(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 5a47ce4343..687a8480e6 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -24327,6 +24327,35 @@
        </para></entry>
       </row>
 
+      <row>
+       <entry role="func_table_entry"><para role="func_signature">
+        <indexterm>
+         <primary>pg_get_owned_sequence</primary>
+        </indexterm>
+        <function>pg_get_owned_sequence</function> ( <parameter>table</parameter> <type>regclass</type>,
<parameter>column</parameter><type>name</type> )
 
+        <returnvalue>regclass</returnvalue>
+       </para>
+       <para>
+        Returns the sequence associated with a column, or NULL if no sequence
+        is associated with the column.  If the column is an identity column,
+        the associated sequence is the sequence internally created for that
+        column.  For columns created using one of the serial types
+        (<type>serial</type>, <type>smallserial</type>, <type>bigserial</type>),
+        it is the sequence created for that serial column definition.  In the
+        latter case, the association can be modified or removed
+        with <command>ALTER SEQUENCE OWNED BY</command>.  The result is
+        suitable for passing to the sequence functions (see
+        <xref linkend="functions-sequence"/>).
+       </para>
+       <para>
+        A typical use is in reading the current value of the sequence for an
+        identity or serial column, for example:
+<programlisting>
+SELECT currval(pg_get_owned_sequence('sometable', 'id'));
+</programlisting>
+       </para></entry>
+      </row>
+
       <row>
        <entry role="func_table_entry"><para role="func_signature">
         <indexterm>
@@ -24336,19 +24365,10 @@
         <returnvalue>text</returnvalue>
        </para>
        <para>
-        Returns the name of the sequence associated with a column,
-        or NULL if no sequence is associated with the column.
-        If the column is an identity column, the associated sequence is the
-        sequence internally created for that column.
-        For columns created using one of the serial types
-        (<type>serial</type>, <type>smallserial</type>, <type>bigserial</type>),
-        it is the sequence created for that serial column definition.
-        In the latter case, the association can be modified or removed
-        with <command>ALTER SEQUENCE OWNED BY</command>.
-        (This function probably should have been
-        called <function>pg_get_owned_sequence</function>; its current name
-        reflects the fact that it has historically been used with serial-type
-        columns.)  The first parameter is a table name with optional
+        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
         schema, and the second parameter is a column name.  Because the first
         parameter potentially contains both schema and table names, it is
         parsed per usual SQL rules, meaning it is lower-cased by default.
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index d3a973d86b..b20a1e7583 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -518,6 +518,7 @@ static char *generate_qualified_type_name(Oid typid);
 static text *string_to_text(char *str);
 static char *flatten_reloptions(Oid relid);
 static void get_reloptions(StringInfo buf, Datum reloptions);
+static Oid pg_get_owned_sequence_internal(Oid tableOid, char *column);
 
 #define only_marker(rte)  ((rte)->inh ? "" : "ONLY ")
 
@@ -2763,6 +2764,28 @@ pg_get_userbyid(PG_FUNCTION_ARGS)
 }
 
 
+/*
+ * pg_get_owned_sequence
+ *        Get the sequence used by an identity or serial column.
+ */
+Datum
+pg_get_owned_sequence(PG_FUNCTION_ARGS)
+{
+    Oid            tableOid = PG_GETARG_OID(0);
+    char       *column = NameStr(*PG_GETARG_NAME(1));
+    Oid            sequenceId;
+
+    sequenceId = pg_get_owned_sequence_internal(tableOid, column);
+
+    if (OidIsValid(sequenceId))
+    {
+        PG_RETURN_OID(sequenceId);
+    }
+
+    PG_RETURN_NULL();
+}
+
+
 /*
  * pg_get_serial_sequence
  *        Get the name of the sequence used by an identity or serial column,
@@ -2778,6 +2801,32 @@ pg_get_serial_sequence(PG_FUNCTION_ARGS)
     RangeVar   *tablerv;
     Oid            tableOid;
     char       *column;
+    Oid            sequenceId;
+
+    /* Look up table name.  Can't lock it - we might not have privileges. */
+    tablerv = makeRangeVarFromNameList(textToQualifiedNameList(tablename));
+    tableOid = RangeVarGetRelid(tablerv, NoLock, false);
+
+    column = text_to_cstring(columnname);
+
+    sequenceId = pg_get_owned_sequence_internal(tableOid, column);
+
+    if (OidIsValid(sequenceId))
+    {
+        char       *result;
+
+        result = generate_qualified_relation_name(sequenceId);
+
+        PG_RETURN_TEXT_P(string_to_text(result));
+    }
+
+    PG_RETURN_NULL();
+}
+
+
+static Oid
+pg_get_owned_sequence_internal(Oid tableOid, char *column)
+{
     AttrNumber    attnum;
     Oid            sequenceId = InvalidOid;
     Relation    depRel;
@@ -2785,19 +2834,13 @@ pg_get_serial_sequence(PG_FUNCTION_ARGS)
     SysScanDesc scan;
     HeapTuple    tup;
 
-    /* Look up table name.  Can't lock it - we might not have privileges. */
-    tablerv = makeRangeVarFromNameList(textToQualifiedNameList(tablename));
-    tableOid = RangeVarGetRelid(tablerv, NoLock, false);
-
     /* Get the number of the column */
-    column = text_to_cstring(columnname);
-
     attnum = get_attnum(tableOid, column);
     if (attnum == InvalidAttrNumber)
         ereport(ERROR,
                 (errcode(ERRCODE_UNDEFINED_COLUMN),
                  errmsg("column \"%s\" of relation \"%s\" does not exist",
-                        column, tablerv->relname)));
+                        column, get_relation_name(tableOid))));
 
     /* Search the dependency table for the dependent sequence */
     depRel = table_open(DependRelationId, AccessShareLock);
@@ -2841,19 +2884,11 @@ pg_get_serial_sequence(PG_FUNCTION_ARGS)
     systable_endscan(scan);
     table_close(depRel, AccessShareLock);
 
-    if (OidIsValid(sequenceId))
-    {
-        char       *result;
-
-        result = generate_qualified_relation_name(sequenceId);
-
-        PG_RETURN_TEXT_P(string_to_text(result));
-    }
-
-    PG_RETURN_NULL();
+    return sequenceId;
 }
 
 
+
 /*
  * pg_get_functiondef
  *        Returns the complete "CREATE OR REPLACE FUNCTION ..." statement for
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 6996073989..34270a4c44 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -3809,6 +3809,9 @@
 { oid => '1716', descr => 'deparse an encoded expression',
   proname => 'pg_get_expr', provolatile => 's', prorettype => 'text',
   proargtypes => 'pg_node_tree oid', prosrc => 'pg_get_expr' },
+{ oid => '8973', descr => 'name of sequence for an identity or serial column',
+  proname => 'pg_get_owned_sequence', provolatile => 's', prorettype => 'regclass',
+  proargtypes => 'regclass name', prosrc => 'pg_get_owned_sequence' },
 { oid => '1665', descr => 'name of sequence for a serial column',
   proname => 'pg_get_serial_sequence', provolatile => 's', prorettype => 'text',
   proargtypes => 'text text', prosrc => 'pg_get_serial_sequence' },
diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out
index 5f03d8e14f..dc8aa102ac 100644
--- a/src/test/regress/expected/identity.out
+++ b/src/test/regress/expected/identity.out
@@ -32,6 +32,12 @@ SELECT pg_get_serial_sequence('itest1', 'a');
  public.itest1_a_seq
 (1 row)
 
+SELECT pg_get_owned_sequence('itest1', 'a');
+ pg_get_owned_sequence 
+-----------------------
+ itest1_a_seq
+(1 row)
+
 \d itest1_a_seq
                     Sequence "public.itest1_a_seq"
   Type   | Start | Minimum |  Maximum   | Increment | Cycles? | Cache 
diff --git a/src/test/regress/expected/sequence.out b/src/test/regress/expected/sequence.out
index 7cb2f7cc02..283fff6a31 100644
--- a/src/test/regress/expected/sequence.out
+++ b/src/test/regress/expected/sequence.out
@@ -84,6 +84,12 @@ SELECT pg_get_serial_sequence('serialTest1', 'f2');
  public.serialtest1_f2_seq
 (1 row)
 
+SELECT pg_get_owned_sequence('serialTest1', 'f2');
+ pg_get_owned_sequence 
+-----------------------
+ serialtest1_f2_seq
+(1 row)
+
 -- test smallserial / bigserial
 CREATE TABLE serialTest2 (f1 text, f2 serial, f3 smallserial, f4 serial2,
   f5 bigserial, f6 serial8);
diff --git a/src/test/regress/sql/identity.sql b/src/test/regress/sql/identity.sql
index 9b8db2e4a3..3d78643b76 100644
--- a/src/test/regress/sql/identity.sql
+++ b/src/test/regress/sql/identity.sql
@@ -13,6 +13,7 @@ SELECT table_name, column_name, column_default, is_nullable, is_identity, identi
 SELECT sequence_name FROM information_schema.sequences WHERE sequence_name LIKE 'itest%';
 
 SELECT pg_get_serial_sequence('itest1', 'a');
+SELECT pg_get_owned_sequence('itest1', 'a');
 
 \d itest1_a_seq
 
diff --git a/src/test/regress/sql/sequence.sql b/src/test/regress/sql/sequence.sql
index 674f5f1f66..828d3ede8b 100644
--- a/src/test/regress/sql/sequence.sql
+++ b/src/test/regress/sql/sequence.sql
@@ -61,6 +61,7 @@ INSERT INTO serialTest1 VALUES ('wrong', NULL);
 SELECT * FROM serialTest1;
 
 SELECT pg_get_serial_sequence('serialTest1', 'f2');
+SELECT pg_get_owned_sequence('serialTest1', 'f2');
 
 -- test smallserial / bigserial
 CREATE TABLE serialTest2 (f1 text, f2 serial, f3 smallserial, f4 serial2,
-- 
2.39.2

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@gmail.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@gmail.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@gmail.com) wrote:
> On Fri, Sep 01, 2023 at 01:31:43PM -0400, Tom Lane wrote:
> > Nathan Bossart <nathandbossart@gmail.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@eisentraut.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@sss.pgh.pa.us> writes:

> Peter Eisentraut <peter@eisentraut.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@sss.pgh.pa.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@ilmari.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@gmail.com> wrote:
>
> On Tue, Sep 12, 2023 at 03:53:28PM +0100, Dagfinn Ilmari Mannsåker wrote:
> > Tom Lane <tgl@sss.pgh.pa.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@gmail.com> writes:

> On Tue, 24 Oct 2023 at 22:00, Nathan Bossart <nathandbossart@gmail.com> wrote:
>>
>> On Tue, Sep 12, 2023 at 03:53:28PM +0100, Dagfinn Ilmari Mannsåker wrote:
>> > Tom Lane <tgl@sss.pgh.pa.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

From 75ed637ec4ed84ac92eb7385fd295b7d5450a450 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Fri, 9 Jun 2023 19:55:58 +0100
Subject: [PATCH v2] Add pg_get_identity_sequence function
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The docs docs say `pg_get_serial_sequence` should have been called
`pg_get_owned_sequence`, but identity columns' sequences are not owned
in the sense of `ALTER SEQUENCE … SET OWNED BY`, so instead call it
`pg_get_identity_sequence`.  This 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`.
---
 doc/src/sgml/func.sgml                 | 37 +++++++++++---
 src/backend/utils/adt/ruleutils.c      | 69 +++++++++++++++++++-------
 src/include/catalog/pg_proc.dat        |  3 ++
 src/test/regress/expected/identity.out |  6 +++
 src/test/regress/expected/sequence.out |  6 +++
 src/test/regress/sql/identity.sql      |  1 +
 src/test/regress/sql/sequence.sql      |  1 +
 7 files changed, 98 insertions(+), 25 deletions(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index cec21e42c0..ff4fa5a0c4 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -24589,13 +24589,13 @@
       <row>
        <entry role="func_table_entry"><para role="func_signature">
         <indexterm>
-         <primary>pg_get_serial_sequence</primary>
+         <primary>pg_get_identity_sequence</primary>
         </indexterm>
-        <function>pg_get_serial_sequence</function> ( <parameter>table</parameter> <type>text</type>,
<parameter>column</parameter><type>text</type> )
 
-        <returnvalue>text</returnvalue>
+        <function>pg_get_identity_sequence</function> ( <parameter>table</parameter> <type>regclass</type>,
<parameter>column</parameter><type>name</type> )
 
+        <returnvalue>regclass</returnvalue>
        </para>
        <para>
-        Returns the name of the sequence associated with a column,
+        Returns the sequence associated with identity or serial column,
         or NULL if no sequence is associated with the column.
         If the column is an identity column, the associated sequence is the
         sequence internally created for that column.
@@ -24604,10 +24604,31 @@
         it is the sequence created for that serial column definition.
         In the latter case, the association can be modified or removed
         with <command>ALTER SEQUENCE OWNED BY</command>.
-        (This function probably should have been
-        called <function>pg_get_owned_sequence</function>; its current name
-        reflects the fact that it has historically been used with serial-type
-        columns.)  The first parameter is a table name with optional
+        The result is suitable for passing to the sequence functions (see
+        <xref linkend="functions-sequence"/>).
+       </para>
+       <para>
+        A typical use is in reading the current value of the sequence for an
+        identity or serial column, for example:
+<programlisting>
+SELECT currval(pg_get_identity_sequence('sometable', 'id'));
+</programlisting>
+       </para></entry>
+      </row>
+
+      <row>
+       <entry role="func_table_entry"><para role="func_signature">
+        <indexterm>
+         <primary>pg_get_serial_sequence</primary>
+        </indexterm>
+        <function>pg_get_serial_sequence</function> ( <parameter>table</parameter> <type>text</type>,
<parameter>column</parameter><type>text</type> )
 
+        <returnvalue>text</returnvalue>
+       </para>
+       <para>
+        A backwards-compatibility wrapper
+        for <function>pg_get_identity_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
         schema, and the second parameter is a column name.  Because the first
         parameter potentially contains both schema and table names, it is
         parsed per usual SQL rules, meaning it is lower-cased by default.
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 0b2a164057..82982be0fe 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -518,6 +518,7 @@ static char *generate_qualified_type_name(Oid typid);
 static text *string_to_text(char *str);
 static char *flatten_reloptions(Oid relid);
 static void get_reloptions(StringInfo buf, Datum reloptions);
+static Oid pg_get_identity_sequence_internal(Oid tableOid, char *column);
 
 #define only_marker(rte)  ((rte)->inh ? "" : "ONLY ")
 
@@ -2777,6 +2778,28 @@ pg_get_userbyid(PG_FUNCTION_ARGS)
 }
 
 
+/*
+ * pg_get_identity_sequence
+ *        Get the sequence used by an identity or serial column.
+ */
+Datum
+pg_get_identity_sequence(PG_FUNCTION_ARGS)
+{
+    Oid            tableOid = PG_GETARG_OID(0);
+    char       *column = NameStr(*PG_GETARG_NAME(1));
+    Oid            sequenceId;
+
+    sequenceId = pg_get_identity_sequence_internal(tableOid, column);
+
+    if (OidIsValid(sequenceId))
+    {
+        PG_RETURN_OID(sequenceId);
+    }
+
+    PG_RETURN_NULL();
+}
+
+
 /*
  * pg_get_serial_sequence
  *        Get the name of the sequence used by an identity or serial column,
@@ -2792,6 +2815,32 @@ pg_get_serial_sequence(PG_FUNCTION_ARGS)
     RangeVar   *tablerv;
     Oid            tableOid;
     char       *column;
+    Oid            sequenceId;
+
+    /* Look up table name.  Can't lock it - we might not have privileges. */
+    tablerv = makeRangeVarFromNameList(textToQualifiedNameList(tablename));
+    tableOid = RangeVarGetRelid(tablerv, NoLock, false);
+
+    column = text_to_cstring(columnname);
+
+    sequenceId = pg_get_identity_sequence_internal(tableOid, column);
+
+    if (OidIsValid(sequenceId))
+    {
+        char       *result;
+
+        result = generate_qualified_relation_name(sequenceId);
+
+        PG_RETURN_TEXT_P(string_to_text(result));
+    }
+
+    PG_RETURN_NULL();
+}
+
+
+static Oid
+pg_get_identity_sequence_internal(Oid tableOid, char *column)
+{
     AttrNumber    attnum;
     Oid            sequenceId = InvalidOid;
     Relation    depRel;
@@ -2799,19 +2848,13 @@ pg_get_serial_sequence(PG_FUNCTION_ARGS)
     SysScanDesc scan;
     HeapTuple    tup;
 
-    /* Look up table name.  Can't lock it - we might not have privileges. */
-    tablerv = makeRangeVarFromNameList(textToQualifiedNameList(tablename));
-    tableOid = RangeVarGetRelid(tablerv, NoLock, false);
-
     /* Get the number of the column */
-    column = text_to_cstring(columnname);
-
     attnum = get_attnum(tableOid, column);
     if (attnum == InvalidAttrNumber)
         ereport(ERROR,
                 (errcode(ERRCODE_UNDEFINED_COLUMN),
                  errmsg("column \"%s\" of relation \"%s\" does not exist",
-                        column, tablerv->relname)));
+                        column, get_relation_name(tableOid))));
 
     /* Search the dependency table for the dependent sequence */
     depRel = table_open(DependRelationId, AccessShareLock);
@@ -2855,19 +2898,11 @@ pg_get_serial_sequence(PG_FUNCTION_ARGS)
     systable_endscan(scan);
     table_close(depRel, AccessShareLock);
 
-    if (OidIsValid(sequenceId))
-    {
-        char       *result;
-
-        result = generate_qualified_relation_name(sequenceId);
-
-        PG_RETURN_TEXT_P(string_to_text(result));
-    }
-
-    PG_RETURN_NULL();
+    return sequenceId;
 }
 
 
+
 /*
  * pg_get_functiondef
  *        Returns the complete "CREATE OR REPLACE FUNCTION ..." statement for
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 7979392776..ceb2acdfff 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -3824,6 +3824,9 @@
 { oid => '1716', descr => 'deparse an encoded expression',
   proname => 'pg_get_expr', provolatile => 's', prorettype => 'text',
   proargtypes => 'pg_node_tree oid', prosrc => 'pg_get_expr' },
+{ oid => '8973', descr => 'name of sequence for an identity or serial column',
+  proname => 'pg_get_identity_sequence', provolatile => 's', prorettype => 'regclass',
+  proargtypes => 'regclass name', prosrc => 'pg_get_identity_sequence' },
 { oid => '1665', descr => 'name of sequence for a serial column',
   proname => 'pg_get_serial_sequence', provolatile => 's', prorettype => 'text',
   proargtypes => 'text text', prosrc => 'pg_get_serial_sequence' },
diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out
index 7c6e87e8a5..48385de647 100644
--- a/src/test/regress/expected/identity.out
+++ b/src/test/regress/expected/identity.out
@@ -32,6 +32,12 @@ SELECT pg_get_serial_sequence('itest1', 'a');
  public.itest1_a_seq
 (1 row)
 
+SELECT pg_get_identity_sequence('itest1', 'a');
+ pg_get_identity_sequence 
+--------------------------
+ itest1_a_seq
+(1 row)
+
 \d itest1_a_seq
                     Sequence "public.itest1_a_seq"
   Type   | Start | Minimum |  Maximum   | Increment | Cycles? | Cache 
diff --git a/src/test/regress/expected/sequence.out b/src/test/regress/expected/sequence.out
index 7cb2f7cc02..e6952ffbae 100644
--- a/src/test/regress/expected/sequence.out
+++ b/src/test/regress/expected/sequence.out
@@ -84,6 +84,12 @@ SELECT pg_get_serial_sequence('serialTest1', 'f2');
  public.serialtest1_f2_seq
 (1 row)
 
+SELECT pg_get_identity_sequence('serialTest1', 'f2');
+ pg_get_identity_sequence 
+--------------------------
+ serialtest1_f2_seq
+(1 row)
+
 -- test smallserial / bigserial
 CREATE TABLE serialTest2 (f1 text, f2 serial, f3 smallserial, f4 serial2,
   f5 bigserial, f6 serial8);
diff --git a/src/test/regress/sql/identity.sql b/src/test/regress/sql/identity.sql
index 9b8db2e4a3..c1ae2fa245 100644
--- a/src/test/regress/sql/identity.sql
+++ b/src/test/regress/sql/identity.sql
@@ -13,6 +13,7 @@ SELECT table_name, column_name, column_default, is_nullable, is_identity, identi
 SELECT sequence_name FROM information_schema.sequences WHERE sequence_name LIKE 'itest%';
 
 SELECT pg_get_serial_sequence('itest1', 'a');
+SELECT pg_get_identity_sequence('itest1', 'a');
 
 \d itest1_a_seq
 
diff --git a/src/test/regress/sql/sequence.sql b/src/test/regress/sql/sequence.sql
index 674f5f1f66..5618a65644 100644
--- a/src/test/regress/sql/sequence.sql
+++ b/src/test/regress/sql/sequence.sql
@@ -61,6 +61,7 @@ INSERT INTO serialTest1 VALUES ('wrong', NULL);
 SELECT * FROM serialTest1;
 
 SELECT pg_get_serial_sequence('serialTest1', 'f2');
+SELECT pg_get_identity_sequence('serialTest1', 'f2');
 
 -- test smallserial / bigserial
 CREATE TABLE serialTest2 (f1 text, f2 serial, f3 smallserial, f4 serial2,
-- 
2.39.2

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@eisentraut.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@sss.pgh.pa.us> writes:

> Peter Eisentraut <peter@eisentraut.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@ilmari.org> writes:
> Tom Lane <tgl@sss.pgh.pa.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