Feedback-ID: i1ff147bf:Fastmail
From: =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org>
To: vignesh C <vignesh21@gmail.com>
Cc: Nathan Bossart <nathandbossart@gmail.com>,  Tom Lane
 <tgl@sss.pgh.pa.us>,  Peter Eisentraut <peter@eisentraut.org>,
  pgsql-hackers@postgresql.org
Subject: Re: Adding a pg_get_owned_sequence function?
References: <87jzwcsasv.fsf@wibble.ilmari.org>
	<4970adf1-cdb0-9ae5-d405-e9c3395d9931@eisentraut.org>
	<2797313.1694529622@sss.pgh.pa.us> <87v8cfo32v.fsf@wibble.ilmari.org>
	<20231024162929.GA871220@nathanxps13>
	<CALDaNm0_nn-LXSJCXQ6EYiGKjVO8fVO8AemgEVZFQPPdx0=fkg@mail.gmail.com>
Date: Mon, 08 Jan 2024 16:58:02 +0000
In-Reply-To: <CALDaNm0_nn-LXSJCXQ6EYiGKjVO8fVO8AemgEVZFQPPdx0=fkg@mail.gmail.com>
	(vignesh C.'s message of "Mon, 8 Jan 2024 10:54:39 +0530")
Message-ID: <87a5pfn4o5.fsf@wibble.ilmari.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Archived-At: <https://www.postgresql.org/message-id/87a5pfn4o5.fsf%40wibble.ilmari.org>
Precedence: bulk

--=-=-=
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

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


--=-=-=
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: inline;
 filename=v2-0001-Add-pg_get_identity_sequence-function.patch
Content-Transfer-Encoding: 8bit

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


--=-=-=--