Subject: Re: Adding a pg_get_owned_sequence function?

From: Dagfinn Ilmari Mannsåker <[email-masked]>
Date: Mon, 08 Jan 2024 16:58:02 +0000

vignesh C <[email-masked]> writes:

> On Tue, 24 Oct 2023 at 22:00, Nathan Bossart <[email-masked]> wrote:
>>
>> On Tue, Sep 12, 2023 at 03:53:28PM +0100, Dagfinn Ilmari Mannsåker wrote:
>> > Tom Lane <[email-masked]> 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?= <[email-masked]>
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

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 8 Jan 2024 15:08:47 -0600

> 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

---

From: Peter Eisentraut <[email-masked]>
Date: Tue, 9 Jan 2024 17:41:14 +0100

On 08.01.24 22:08, Nathan Bossart wrote:
> both work for identity_and_  serial.  The real differences between the two

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?

---

From: Tom Lane <[email-masked]>
Date: Tue, 09 Jan 2024 12:03:06 -0500

> 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

---

From: Dagfinn Ilmari Mannsåker <[email-masked]>
Date: Tue, 09 Jan 2024 17:44:13 +0000

> 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.

> 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?

- ilmari

[1]: https://postgr.es/m/[email-masked]

---

From: Tom Lane <[email-masked]>
Date: Tue, 09 Jan 2024 15:51:06 -0500

> I think it would be more correct to use name for the column argument

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.

> 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