public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 5/6] Documentation for slot-limit feature
31+ messages / 8 participants
[nested] [flat]
* [PATCH 5/6] Documentation for slot-limit feature
@ 2018-01-11 06:00 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 31+ messages in thread
From: Kyotaro Horiguchi @ 2018-01-11 06:00 UTC (permalink / raw)
---
doc/src/sgml/catalogs.sgml | 28 ++++++++++++++++++++++++++++
doc/src/sgml/config.sgml | 23 +++++++++++++++++++++++
doc/src/sgml/high-availability.sgml | 8 +++++---
3 files changed, 56 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index af4d0625ea..7ec8764ce5 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -9825,6 +9825,34 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
</entry>
</row>
+ <row>
+ <entry><structfield>wal_status</structfield></entry>
+ <entry><type>text</type></entry>
+ <entry></entry>
+
+ <entry>Availability of WAL records claimed by this
+ slot. <literal>streaming</literal>, <literal>keeping</literal>,
+ <literal>lost</literal>
+ or <literal>unknown</literal>. <literal>streaming</literal> means that
+ the claimed records are available. <literal>keeping</literal> means that
+ some of them are to be removed by the next checkpoint.
+ <literal>lost</literal> means that some of them are no longer
+ available. The last two states are seen only when
+ <xref linkend="guc-max-slot-wal-keep-size"/> is
+ non-negative. If <structfield>restart_lsn</structfield> is NULL, this
+ field is <literal>unknown</literal>.
+ </entry>
+ </row>
+
+ <row>
+ <entry><structfield>remain</structfield></entry>
+ <entry><type>bigint</type></entry>
+ <entry></entry>
+ <entry>The amount in bytes that WAL location (LSN) can advance until
+ this slot may lose required WAL records.
+ </entry>
+ </row>
+
</tbody>
</tgroup>
</table>
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index b6f5822b84..7177c6122a 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3537,6 +3537,29 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows
</listitem>
</varlistentry>
+ <varlistentry id="guc-max-slot-wal-keep-size" xreflabel="max_slot_wal_keep_size">
+ <term><varname>max_slot_wal_keep_size</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>max_slot_wal_keep_size</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Specify the maximum size of WAL files
+ that <link linkend="streaming-replication-slots">replication
+ slots</link> are allowed to retain in the <filename>pg_wal</filename>
+ directory at checkpoint time.
+ If <varname>max_slot_wal_keep_size</varname> is -1 (the default),
+ replication slots retain unlimited size of WAL files. If restart_lsn
+ of a replication slot gets behind more than that bytes from the
+ current LSN, the standby using the slot may no longer be able to
+ reconnect due to removal of required WAL records. You can see the WAL
+ availability of replication slots
+ in <link linkend="view-pg-replication-slots">pg_replication_slots</link>.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-wal-sender-timeout" xreflabel="wal_sender_timeout">
<term><varname>wal_sender_timeout</varname> (<type>integer</type>)
<indexterm>
diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index bbab7395a2..79901c5f06 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -925,9 +925,11 @@ primary_conninfo = 'host=192.168.1.50 port=5432 user=foo password=foopass'
<xref linkend="guc-archive-command"/>.
However, these methods often result in retaining more WAL segments than
required, whereas replication slots retain only the number of segments
- known to be needed. An advantage of these methods is that they bound
- the space requirement for <literal>pg_wal</literal>; there is currently no way
- to do this using replication slots.
+ known to be needed. On the other hand, replication slots can retain so
+ many WAL segments that they fill up the space allocated
+ for <literal>pg_wal</literal>;
+ <xref linkend="guc-max-slot-wal-keep-size"/> limits the size of WAL files
+ retained by replication slots.
</para>
<para>
Similarly, <xref linkend="guc-hot-standby-feedback"/>
--
2.16.3
----Next_Part(Wed_Jan_30_10_42_04_2019_681)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v12-0006-Check-removal-of-in-reading-segment-file.patch"
^ permalink raw reply [nested|flat] 31+ messages in thread
* [PATCH 4/4] Documentation for slot-limit feature
@ 2018-01-11 06:00 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 31+ messages in thread
From: Kyotaro Horiguchi @ 2018-01-11 06:00 UTC (permalink / raw)
---
doc/src/sgml/catalogs.sgml | 28 ++++++++++++++++++++++++++++
doc/src/sgml/config.sgml | 23 +++++++++++++++++++++++
doc/src/sgml/high-availability.sgml | 8 +++++---
3 files changed, 56 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 0179deea2e..84a937e1fe 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -9879,6 +9879,34 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
</entry>
</row>
+ <row>
+ <entry><structfield>wal_status</structfield></entry>
+ <entry><type>text</type></entry>
+ <entry></entry>
+
+ <entry>Availability of WAL records claimed by the
+ slot. <literal>streaming</literal>, <literal>keeping</literal>,
+ <literal>lost</literal>
+ or <literal>unknown</literal>. <literal>streaming</literal> means that
+ the claimed records are available. <literal>keeping</literal> means that
+ some of them are to be removed by the next checkpoint.
+ <literal>lost</literal> means that some of them are no longer
+ available. The last two states are seen only when
+ <xref linkend="guc-max-slot-wal-keep-size"/> is non-negative. If the
+ slot doesn't have valid restart_lsn, this field
+ is <literal>unknown</literal>.
+ </entry>
+ </row>
+
+ <row>
+ <entry><structfield>remain</structfield></entry>
+ <entry><type>bigint</type></entry>
+ <entry></entry>
+ <entry>The amount in bytes that WAL location (LSN) can advance until the
+ slot may lose required WAL records.
+ </entry>
+ </row>
+
</tbody>
</tgroup>
</table>
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index bee4afbe4e..edd5419ec6 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3116,6 +3116,29 @@ include_dir 'conf.d'
</listitem>
</varlistentry>
+ <varlistentry id="guc-max-slot-wal-keep-size" xreflabel="max_slot_wal_keep_size">
+ <term><varname>max_slot_wal_keep_size</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>max_slot_wal_keep_size</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Specify the maximum size of WAL files
+ that <link linkend="streaming-replication-slots">replication
+ slots</link> are allowed to retain in the <filename>pg_wal</filename>
+ directory at checkpoint time.
+ If <varname>max_slot_wal_keep_size</varname> is -1 (the default),
+ replication slots retain unlimited size of WAL files. If restart_lsn
+ of a replication slot gets behind more than that bytes from the
+ current LSN, the standby using the slot may no longer be able to
+ reconnect due to removal of required WAL records. You can see the WAL
+ availability of replication slots
+ in <link linkend="view-pg-replication-slots">pg_replication_slots</link>.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-wal-sender-timeout" xreflabel="wal_sender_timeout">
<term><varname>wal_sender_timeout</varname> (<type>integer</type>)
<indexterm>
diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index 8cb77f85ec..04cdccb10d 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -927,9 +927,11 @@ primary_conninfo = 'host=192.168.1.50 port=5432 user=foo password=foopass'
<xref linkend="guc-archive-command"/>.
However, these methods often result in retaining more WAL segments than
required, whereas replication slots retain only the number of segments
- known to be needed. An advantage of these methods is that they bound
- the space requirement for <literal>pg_wal</literal>; there is currently no way
- to do this using replication slots.
+ known to be needed. On the other hand, replication slots can retain so
+ many WAL segments that they fill up the space allotted
+ for <literal>pg_wal</literal>;
+ <xref linkend="guc-max-slot-wal-keep-size"/> limits the size of WAL files
+ retained by replication slots.
</para>
<para>
Similarly, <xref linkend="guc-hot-standby-feedback"/>
--
2.16.3
----Next_Part(Thu_Sep_13_18_29_31_2018_032)----
^ permalink raw reply [nested|flat] 31+ messages in thread
* [PATCH 4/4] Documentation for slot-limit feature
@ 2018-01-11 06:00 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 31+ messages in thread
From: Kyotaro Horiguchi @ 2018-01-11 06:00 UTC (permalink / raw)
---
doc/src/sgml/catalogs.sgml | 28 ++++++++++++++++++++++++++++
doc/src/sgml/config.sgml | 22 ++++++++++++++++++++++
doc/src/sgml/high-availability.sgml | 8 +++++---
3 files changed, 55 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 07e8b3325f..7e31267d68 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -9887,6 +9887,34 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
</entry>
</row>
+ <row>
+ <entry><structfield>wal_status</structfield></entry>
+ <entry><type>text</type></entry>
+ <entry></entry>
+
+ <entry>Availability of WAL records claimed by the
+ slot. <literal>streaming</literal>, <literal>keeping</literal>,
+ <literal>lost</literal>
+ or <literal>unknown</literal>. <literal>streaming</literal> means that
+ the claimed records are available. <literal>keeping</literal> means that
+ some of them are to be removed by the next checkpoint.
+ <literal>lost</literal> means that some of them have been removed. The
+ last two states are seen only when
+ <xref linkend="guc-max-slot-wal-keep-size"/> is not zero. If the slot
+ doesn't have valid restart_lsn, this field
+ is <literal>unknown</literal>.
+ </entry>
+ </row>
+
+ <row>
+ <entry><structfield>remain</structfield></entry>
+ <entry><type>bigint</type></entry>
+ <entry></entry>
+ <entry>The amount in bytes that WAL location (LSN) can advance until the
+ slot may lose required WAL records.
+ </entry>
+ </row>
+
</tbody>
</tgroup>
</table>
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index bee4afbe4e..9d190c3daa 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3116,6 +3116,28 @@ include_dir 'conf.d'
</listitem>
</varlistentry>
+ <varlistentry id="guc-max-slot-wal-keep-size" xreflabel="max_slot_wal_keep_size">
+ <term><varname>max_slot_wal_keep_size</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>max_slot_wal_keep_size</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Specify the maximum size of WAL files
+ that <link linkend="streaming-replication-slots">replication
+ slots</link> are allowed to retain in the <filename>pg_wal</filename>
+ directory at checkpoint time.
+ If <varname>max_slot_wal_keep_size</varname> is zero (the default),
+ replication slots retain unlimited size of WAL files.
+ </para>
+ <para>
+ This parameter is used being rounded down to the multiples of WAL file
+ size.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-wal-sender-timeout" xreflabel="wal_sender_timeout">
<term><varname>wal_sender_timeout</varname> (<type>integer</type>)
<indexterm>
diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index 8cb77f85ec..04cdccb10d 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -927,9 +927,11 @@ primary_conninfo = 'host=192.168.1.50 port=5432 user=foo password=foopass'
<xref linkend="guc-archive-command"/>.
However, these methods often result in retaining more WAL segments than
required, whereas replication slots retain only the number of segments
- known to be needed. An advantage of these methods is that they bound
- the space requirement for <literal>pg_wal</literal>; there is currently no way
- to do this using replication slots.
+ known to be needed. On the other hand, replication slots can retain so
+ many WAL segments that they fill up the space allotted
+ for <literal>pg_wal</literal>;
+ <xref linkend="guc-max-slot-wal-keep-size"/> limits the size of WAL files
+ retained by replication slots.
</para>
<para>
Similarly, <xref linkend="guc-hot-standby-feedback"/>
--
2.16.3
----Next_Part(Tue_Sep_04_19_52_50_2018_992)----
^ permalink raw reply [nested|flat] 31+ messages in thread
* [PATCH 5/6] Documentation for slot-limit feature
@ 2018-01-11 06:00 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 31+ messages in thread
From: Kyotaro Horiguchi @ 2018-01-11 06:00 UTC (permalink / raw)
---
doc/src/sgml/catalogs.sgml | 28 ++++++++++++++++++++++++++++
doc/src/sgml/config.sgml | 23 +++++++++++++++++++++++
doc/src/sgml/high-availability.sgml | 8 +++++---
3 files changed, 56 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index af4d0625ea..7ec8764ce5 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -9825,6 +9825,34 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
</entry>
</row>
+ <row>
+ <entry><structfield>wal_status</structfield></entry>
+ <entry><type>text</type></entry>
+ <entry></entry>
+
+ <entry>Availability of WAL records claimed by this
+ slot. <literal>streaming</literal>, <literal>keeping</literal>,
+ <literal>lost</literal>
+ or <literal>unknown</literal>. <literal>streaming</literal> means that
+ the claimed records are available. <literal>keeping</literal> means that
+ some of them are to be removed by the next checkpoint.
+ <literal>lost</literal> means that some of them are no longer
+ available. The last two states are seen only when
+ <xref linkend="guc-max-slot-wal-keep-size"/> is
+ non-negative. If <structfield>restart_lsn</structfield> is NULL, this
+ field is <literal>unknown</literal>.
+ </entry>
+ </row>
+
+ <row>
+ <entry><structfield>remain</structfield></entry>
+ <entry><type>bigint</type></entry>
+ <entry></entry>
+ <entry>The amount in bytes that WAL location (LSN) can advance until
+ this slot may lose required WAL records.
+ </entry>
+ </row>
+
</tbody>
</tgroup>
</table>
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 4a7121a51f..3d034ac0d1 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3531,6 +3531,29 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows
</listitem>
</varlistentry>
+ <varlistentry id="guc-max-slot-wal-keep-size" xreflabel="max_slot_wal_keep_size">
+ <term><varname>max_slot_wal_keep_size</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>max_slot_wal_keep_size</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Specify the maximum size of WAL files
+ that <link linkend="streaming-replication-slots">replication
+ slots</link> are allowed to retain in the <filename>pg_wal</filename>
+ directory at checkpoint time.
+ If <varname>max_slot_wal_keep_size</varname> is -1 (the default),
+ replication slots retain unlimited size of WAL files. If restart_lsn
+ of a replication slot gets behind more than that bytes from the
+ current LSN, the standby using the slot may no longer be able to
+ reconnect due to removal of required WAL records. You can see the WAL
+ availability of replication slots
+ in <link linkend="view-pg-replication-slots">pg_replication_slots</link>.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-wal-sender-timeout" xreflabel="wal_sender_timeout">
<term><varname>wal_sender_timeout</varname> (<type>integer</type>)
<indexterm>
diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index d8fd195da0..e30eaaeebe 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -925,9 +925,11 @@ primary_conninfo = 'host=192.168.1.50 port=5432 user=foo password=foopass'
<xref linkend="guc-archive-command"/>.
However, these methods often result in retaining more WAL segments than
required, whereas replication slots retain only the number of segments
- known to be needed. An advantage of these methods is that they bound
- the space requirement for <literal>pg_wal</literal>; there is currently no way
- to do this using replication slots.
+ known to be needed. On the other hand, replication slots can retain so
+ many WAL segments that they fill up the space allocated
+ for <literal>pg_wal</literal>;
+ <xref linkend="guc-max-slot-wal-keep-size"/> limits the size of WAL files
+ retained by replication slots.
</para>
<para>
Similarly, <xref linkend="guc-hot-standby-feedback"/>
--
2.16.3
----Next_Part(Thu_Dec_20_16_24_38_2018_792)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v11-0006-Check-removal-of-in-reading-segment-file.patch"
^ permalink raw reply [nested|flat] 31+ messages in thread
* [PATCH 4/4] Documentation for slot-limit feature
@ 2018-01-11 06:00 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 31+ messages in thread
From: Kyotaro Horiguchi @ 2018-01-11 06:00 UTC (permalink / raw)
---
doc/src/sgml/catalogs.sgml | 28 ++++++++++++++++++++++++++++
doc/src/sgml/config.sgml | 23 +++++++++++++++++++++++
doc/src/sgml/high-availability.sgml | 8 +++++---
3 files changed, 56 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 9edba96fab..fc4cbc9239 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -9881,6 +9881,34 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
</entry>
</row>
+ <row>
+ <entry><structfield>wal_status</structfield></entry>
+ <entry><type>text</type></entry>
+ <entry></entry>
+
+ <entry>Availability of WAL records claimed by the
+ slot. <literal>streaming</literal>, <literal>keeping</literal>,
+ <literal>lost</literal>
+ or <literal>unknown</literal>. <literal>streaming</literal> means that
+ the claimed records are available. <literal>keeping</literal> means that
+ some of them are to be removed by the next checkpoint.
+ <literal>lost</literal> means that some of them are no longer
+ available. The last two states are seen only when
+ <xref linkend="guc-max-slot-wal-keep-size"/> is non-negative. If the
+ slot doesn't have valid restart_lsn, this field
+ is <literal>unknown</literal>.
+ </entry>
+ </row>
+
+ <row>
+ <entry><structfield>remain</structfield></entry>
+ <entry><type>bigint</type></entry>
+ <entry></entry>
+ <entry>The amount in bytes that WAL location (LSN) can advance until the
+ slot may lose required WAL records.
+ </entry>
+ </row>
+
</tbody>
</tgroup>
</table>
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 7554cba3f9..f3e504862c 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3117,6 +3117,29 @@ include_dir 'conf.d'
</listitem>
</varlistentry>
+ <varlistentry id="guc-max-slot-wal-keep-size" xreflabel="max_slot_wal_keep_size">
+ <term><varname>max_slot_wal_keep_size</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>max_slot_wal_keep_size</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Specify the maximum size of WAL files
+ that <link linkend="streaming-replication-slots">replication
+ slots</link> are allowed to retain in the <filename>pg_wal</filename>
+ directory at checkpoint time.
+ If <varname>max_slot_wal_keep_size</varname> is -1 (the default),
+ replication slots retain unlimited size of WAL files. If restart_lsn
+ of a replication slot gets behind more than that bytes from the
+ current LSN, the standby using the slot may no longer be able to
+ reconnect due to removal of required WAL records. You can see the WAL
+ availability of replication slots
+ in <link linkend="view-pg-replication-slots">pg_replication_slots</link>.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-wal-sender-timeout" xreflabel="wal_sender_timeout">
<term><varname>wal_sender_timeout</varname> (<type>integer</type>)
<indexterm>
diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index ebcb3daaed..15a98340a6 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -927,9 +927,11 @@ primary_conninfo = 'host=192.168.1.50 port=5432 user=foo password=foopass'
<xref linkend="guc-archive-command"/>.
However, these methods often result in retaining more WAL segments than
required, whereas replication slots retain only the number of segments
- known to be needed. An advantage of these methods is that they bound
- the space requirement for <literal>pg_wal</literal>; there is currently no way
- to do this using replication slots.
+ known to be needed. On the other hand, replication slots can retain so
+ many WAL segments that they fill up the space allotted
+ for <literal>pg_wal</literal>;
+ <xref linkend="guc-max-slot-wal-keep-size"/> limits the size of WAL files
+ retained by replication slots.
</para>
<para>
Similarly, <xref linkend="guc-hot-standby-feedback"/>
--
2.16.3
----Next_Part(Thu_Oct_25_21_55_18_2018_318)----
^ permalink raw reply [nested|flat] 31+ messages in thread
* [PATCH v10 9/9] pg_ls_*dir to return all the metadata from pg_stat_file..
@ 2020-03-10 02:56 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 31+ messages in thread
From: Justin Pryzby @ 2020-03-10 02:56 UTC (permalink / raw)
..but it doesn't seem worth factoring out the common bits, since stat_file
doesn't return a name, so all the field numbers are off by one.
NOTE, the atime is now shown where the mtime used to be.
Need catversion bump
---
doc/src/sgml/func.sgml | 30 +++++++++-----------
src/backend/utils/adt/genfile.c | 17 ++++++++---
src/include/catalog/pg_proc.dat | 30 ++++++++++----------
src/test/regress/expected/misc_functions.out | 8 +++---
src/test/regress/output/tablespace.source | 8 +++---
5 files changed, 49 insertions(+), 44 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index d0b782d803..ae6c004a6d 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -21348,8 +21348,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
</entry>
<entry><type>setof record</type></entry>
<entry>
- For each file in the log directory, list the file's name, size, last
- modification time, and a boolean indicating if it is a directory.
+ For each file in the log directory, list the file and its metadata.
Access is granted to members of the <literal>pg_monitor</literal>
role and may be granted to other non-superuser roles.
</entry>
@@ -21360,8 +21359,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
</entry>
<entry><type>setof record</type></entry>
<entry>
- For each file in the WAL directory, list the file's name, size, last
- modification time, and a boolean indicating if it is a directory.
+ For each file in the WAL directory, list the file and its metadata.
Access is granted to members of the <literal>pg_monitor</literal>
role and may be granted to other non-superuser roles.
</entry>
@@ -21372,9 +21370,8 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
</entry>
<entry><type>setof record</type></entry>
<entry>
- For each file in the WAL archive status directory, list the file's
- name, size, last modification time, and a boolean indicating if it is a
- directory. Access is granted to members of the
+ For each file in the WAL archive status directory, list the file and its metadata.
+ Access is granted to members of the
<literal>pg_monitor</literal> role and may be granted to other
non-superuser roles.
</entry>
@@ -21386,8 +21383,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
<entry><type>setof record</type></entry>
<entry>
For the temporary directory within <parameter>tablespace</parameter>,
- list each file's name, size, last modification time, and a boolean
- indicating if it is a directory. If <parameter>tablespace</parameter>
+ list each file and its metadata. If <parameter>tablespace</parameter>
is not provided, the <literal>pg_default</literal> tablespace is used.
Access is granted to members of the <literal>pg_monitor</literal> role
and may be granted to other non-superuser roles.
@@ -21463,8 +21459,8 @@ SELECT * FROM (SELECT DISTINCT COALESCE(NULLIF(pg_tablespace_location(b.oid),'')
</indexterm>
<para>
<function>pg_ls_logdir</function> lists each file in the log directory,
- along with file's size, last modification time, and a boolean
- indicating if the file is a directory. By default, only superusers
+ along with the metadata columns returned by <function>pg_stat_file</function>.
+ By default, only superusers
and members of the <literal>pg_monitor</literal> role can use this function.
Access may be granted to others using <command>GRANT</command>.
Filenames beginning with a dot and special file types are not shown.
@@ -21475,8 +21471,8 @@ SELECT * FROM (SELECT DISTINCT COALESCE(NULLIF(pg_tablespace_location(b.oid),'')
</indexterm>
<para>
<function>pg_ls_waldir</function> lists each file in the WAL directory,
- along with the file's size, last modification time, and a boolean
- indicating if the file is a directory. By default, only superusers
+ along with the metadata columns returned by <function>pg_stat_file</function>.
+ By default, only superusers
and members of the <literal>pg_monitor</literal> role
can use this function. Access may be granted to others using
<command>GRANT</command>.
@@ -21488,8 +21484,8 @@ SELECT * FROM (SELECT DISTINCT COALESCE(NULLIF(pg_tablespace_location(b.oid),'')
</indexterm>
<para>
<function>pg_ls_archive_statusdir</function> lists each file in the WAL
- archive status directory, along with the file's size, last modification
- time, and a boolean indicating if the file is a directory. By default, only
+ archive status directory, along with the metadata columns returned by
+ <function>pg_stat_file</function>. By default, only
superusers and members of the <literal>pg_monitor</literal> role can
use this function. Access may be granted to others using
<command>GRANT</command>.
@@ -21502,8 +21498,8 @@ SELECT * FROM (SELECT DISTINCT COALESCE(NULLIF(pg_tablespace_location(b.oid),'')
<para>
<function>pg_ls_tmpdir</function> lists each file in the temporary file
directory for the specified <parameter>tablespace</parameter>, along with
- its size, last modified time (mtime) and a boolean indicating if the file is a
- directory. Directories are used for temporary files shared by parallel
+ the metadata columns returned by <function>pg_stat_file</function>.
+ Directories are used for temporary files shared by parallel
processes. If <parameter>tablespace</parameter> is
not provided, the <literal>pg_default</literal> tablespace is used. By
default only superusers and members of the <literal>pg_monitor</literal>
diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
index 19e0b8a82c..fb6cdc07a7 100644
--- a/src/backend/utils/adt/genfile.c
+++ b/src/backend/utils/adt/genfile.c
@@ -559,8 +559,8 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags)
while ((de = ReadDir(dirdesc, dir)) != NULL)
{
- Datum values[4];
- bool nulls[4];
+ Datum values[7];
+ bool nulls[7];
char path[MAXPGPATH * 2];
struct stat attrib;
@@ -593,8 +593,17 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags)
if (flags & FLAG_METADATA)
{
values[1] = Int64GetDatum((int64) attrib.st_size);
- values[2] = TimestampTzGetDatum(time_t_to_timestamptz(attrib.st_mtime));
- values[3] = BoolGetDatum(S_ISDIR(attrib.st_mode));
+ values[2] = TimestampTzGetDatum(time_t_to_timestamptz(attrib.st_atime));
+ values[3] = TimestampTzGetDatum(time_t_to_timestamptz(attrib.st_mtime));
+ /* Unix has file status change time, while Win32 has creation time */
+#if !defined(WIN32) && !defined(__CYGWIN__)
+ values[4] = TimestampTzGetDatum(time_t_to_timestamptz(attrib.st_ctime));
+ nulls[5] = true;
+#else
+ nulls[4] = true;
+ values[5] = TimestampTzGetDatum(time_t_to_timestamptz(attrib.st_ctime));
+#endif
+ values[6] = BoolGetDatum(S_ISDIR(attrib.st_mode));
}
memset(nulls, 0, sizeof(nulls));
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 75423db22e..6c11aa940e 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6147,9 +6147,9 @@
{ oid => '8511', descr => 'list all files in a directory recursively',
proname => 'pg_ls_dir_recurse', prorows => '10000', proretset => 't',
provolatile => 'v', prorettype => 'record', proargtypes => 'text',
- proallargtypes => '{text,text,int8,timestamptz,bool}',
- proargnames => '{dirname,name,size,modification,isdir}', proargmodes => '{i,o,o,o,o}',
- prolang => 'sql', prosrc => "WITH RECURSIVE x AS (SELECT * FROM pg_ls_dir_metadata(dirname, true, false, true) UNION ALL SELECT x.name||'/'||a.name, a.size, a.modification, a.isdir FROM x, pg_ls_dir_metadata(dirname||'/'||x.name, true, false, true)a WHERE x.isdir) SELECT * FROM x" },
+ proallargtypes => '{text,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}',
+ proargnames => '{dirname,name,size,access,modification,change,creation,isdir}', proargmodes => '{i,o,o,o,o,o,o,o}',
+ prolang => 'sql', prosrc => "WITH RECURSIVE x AS (SELECT * FROM pg_ls_dir_metadata(dirname, true, false) UNION ALL SELECT x.name||'/'||a.name, a.size, a.access, a.modification,a.change,a.creation,a.isdir FROM x, pg_ls_dir_metadata(dirname||'/'||x.name, true, false)a WHERE x.isdir) SELECT * FROM x" },
{ oid => '2626', descr => 'sleep for the specified time in seconds',
proname => 'pg_sleep', provolatile => 'v', prorettype => 'void',
@@ -10725,35 +10725,35 @@
{ oid => '3353', descr => 'list files in the log directory',
proname => 'pg_ls_logdir', procost => '10', prorows => '20', proretset => 't',
provolatile => 'v', prorettype => 'record', proargtypes => '',
- proallargtypes => '{text,int8,timestamptz,bool}', proargmodes => '{o,o,o,o}',
- proargnames => '{name,size,modification,isdir}', prosrc => 'pg_ls_logdir' },
+ proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{o,o,o,o,o,o,o}',
+ proargnames => '{name,size,access,modification,change,creation,isdir}', prosrc => 'pg_ls_logdir' },
{ oid => '3354', descr => 'list of files in the WAL directory',
proname => 'pg_ls_waldir', procost => '10', prorows => '20', proretset => 't',
provolatile => 'v', prorettype => 'record', proargtypes => '',
- proallargtypes => '{text,int8,timestamptz,bool}', proargmodes => '{o,o,o,o}',
- proargnames => '{name,size,modification,isdir}', prosrc => 'pg_ls_waldir' },
+ proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{o,o,o,o,o,o,o}',
+ proargnames => '{name,size,access,modification,change,creation,isdir}', prosrc => 'pg_ls_waldir' },
{ oid => '5031', descr => 'list of files in the archive_status directory',
proname => 'pg_ls_archive_statusdir', procost => '10', prorows => '20',
proretset => 't', provolatile => 'v', prorettype => 'record',
- proargtypes => '', proallargtypes => '{text,int8,timestamptz,bool}',
- proargmodes => '{o,o,o,o}', proargnames => '{name,size,modification,isdir}',
+ proargtypes => '', proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}',
+ proargmodes => '{o,o,o,o,o,o,o}', proargnames => '{name,size,access,modification,change,creation,isdir}',
prosrc => 'pg_ls_archive_statusdir' },
{ oid => '5029', descr => 'list files in the pgsql_tmp directory',
proname => 'pg_ls_tmpdir', procost => '10', prorows => '20', proretset => 't',
provolatile => 'v', prorettype => 'record', proargtypes => '',
- proallargtypes => '{text,int8,timestamptz,bool}', proargmodes => '{o,o,o,o}',
- proargnames => '{name,size,modification,isdir}', prosrc => 'pg_ls_tmpdir_noargs' },
+ proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{o,o,o,o,o,o,o}',
+ proargnames => '{name,size,access,modification,change,creation,isdir}', prosrc => 'pg_ls_tmpdir_noargs' },
{ oid => '5030', descr => 'list files in the pgsql_tmp directory',
proname => 'pg_ls_tmpdir', procost => '10', prorows => '20', proretset => 't',
provolatile => 'v', prorettype => 'record', proargtypes => 'oid',
- proallargtypes => '{oid,text,int8,timestamptz,bool}', proargmodes => '{i,o,o,o,o}',
- proargnames => '{tablespace,name,size,modification,isdir}',
+ proallargtypes => '{oid,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{i,o,o,o,o,o,o,o}',
+ proargnames => '{tablespace,name,size,access,modification,change,creation,isdir}',
prosrc => 'pg_ls_tmpdir_1arg' },
{ oid => '5032', descr => 'list directory with metadata',
proname => 'pg_ls_dir_metadata', procost => '10', prorows => '20', proretset => 't',
provolatile => 'v', prorettype => 'record', proargtypes => 'text bool bool',
- proallargtypes => '{text,bool,bool,text,int8,timestamptz,bool}', proargmodes => '{i,i,i,o,o,o,o}',
- proargnames => '{dirname,missing_ok,include_dot_dirs,name,size,modification,isdir}',
+ proallargtypes => '{text,bool,bool,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{i,i,i,o,o,o,o,o,o,o}',
+ proargnames => '{dirname,missing_ok,include_dot_dirs,name,size,access,modification,change,creation,isdir}',
prosrc => 'pg_ls_dir_metadata' },
# hash partitioning constraint function
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index d11b8669c3..ea7056cd5c 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -157,8 +157,8 @@ select count(*) > 0 as ok from (select pg_ls_waldir()) ss;
-- Test not-run-to-completion cases.
select * from pg_ls_waldir() limit 0;
- name | size | modification | isdir
-------+------+--------------+-------
+ name | size | access | modification | change | creation | isdir
+------+------+--------+--------------+--------+----------+-------
(0 rows)
select count(*) > 0 as ok from (select * from pg_ls_waldir() limit 1) ss;
@@ -182,8 +182,8 @@ select count(*) >= 0 as ok from pg_ls_archive_statusdir();
-- This tests the missing_ok parameter, which causes pg_ls_tmpdir to succeed even if the tmpdir doesn't exist yet
-- The name='' condition is never true, so the function runs to completion but returns zero rows.
select * from pg_ls_tmpdir() where name='Does not exist';
- name | size | modification | isdir
-------+------+--------------+-------
+ name | size | access | modification | change | creation | isdir
+------+------+--------+--------------+--------+----------+-------
(0 rows)
--
diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source
index dacaafcae6..1e1e02b589 100644
--- a/src/test/regress/output/tablespace.source
+++ b/src/test/regress/output/tablespace.source
@@ -17,15 +17,15 @@ CREATE TABLESPACE regress_tblspace LOCATION '@testtablespace@';
-- The name='' condition is never true, so the function runs to completion but returns zero rows.
-- The query is written to ERROR if the tablespace doesn't exist, rather than silently failing to call pg_ls_tmpdir()
SELECT c.* FROM (SELECT oid FROM pg_tablespace b WHERE b.spcname='regress_tblspace' UNION SELECT 0 ORDER BY 1 DESC LIMIT 1) AS b , pg_ls_tmpdir(oid) AS c WHERE c.name='Does not exist';
- name | size | modification | isdir
-------+------+--------------+-------
+ name | size | access | modification | change | creation | isdir
+------+------+--------+--------------+--------+----------+-------
(0 rows)
-- This tests the missing_ok parameter. If that's not functioning, this would ERROR if the logdir doesn't exist yet.
-- The name='' condition is never true, so the function runs to completion but returns zero rows.
SELECT * FROM pg_ls_logdir() WHERE name='Does not exist';
- name | size | modification
-------+------+--------------
+ name | size | access | modification | change | creation | isdir
+------+------+--------+--------------+--------+----------+-------
(0 rows)
-- try setting and resetting some properties for the new tablespace
--
2.17.0
--rCwQ2Y43eQY6RBgR--
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-01-16 18:59 Peter Eisentraut <[email protected]>
0 siblings, 2 replies; 31+ messages in thread
From: Peter Eisentraut @ 2024-01-16 18:59 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On 09.01.24 15:10, Ashutosh Bapat wrote:
> Here's complete patch-set.
Looks good! Committed.
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-01-17 05:36 Ashutosh Bapat <[email protected]>
parent: Peter Eisentraut <[email protected]>
1 sibling, 1 reply; 31+ messages in thread
From: Ashutosh Bapat @ 2024-01-17 05:36 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Wed, Jan 17, 2024 at 12:30 AM Peter Eisentraut <[email protected]> wrote:
>
> On 09.01.24 15:10, Ashutosh Bapat wrote:
> > Here's complete patch-set.
>
> Looks good! Committed.
>
Thanks a lot Peter.
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-01-22 12:02 Peter Eisentraut <[email protected]>
parent: Ashutosh Bapat <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Peter Eisentraut @ 2024-01-22 12:02 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On 17.01.24 06:36, Ashutosh Bapat wrote:
> On Wed, Jan 17, 2024 at 12:30 AM Peter Eisentraut <[email protected]> wrote:
>>
>> On 09.01.24 15:10, Ashutosh Bapat wrote:
>>> Here's complete patch-set.
>>
>> Looks good! Committed.
>>
>
> Thanks a lot Peter.
I found another piece of code that might need updating, or at least the
comment.
In MergeAttributes(), in the part that merges the specified column
definitions into the inherited ones, it says
/*
* Identity is never inherited. The new column can have an
* identity definition, so we always just take that one.
*/
def->identity = newdef->identity;
This is still correct for regular inheritance, but not for partitioning.
I think for partitioning, this is not reachable because you can't
specify identity information when you create a partition(?). So maybe
something like
if (newdef->identity)
{
Assert(!is_partioning);
/*
* Identity is never inherited. The new column can have an
* identity definition, so we always just take that one.
*/
def->identity = newdef->identity;
}
Thoughts?
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-01-22 12:23 Ashutosh Bapat <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Ashutosh Bapat @ 2024-01-22 12:23 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Mon, Jan 22, 2024 at 5:32 PM Peter Eisentraut <[email protected]> wrote:
>
> I found another piece of code that might need updating, or at least the
> comment.
>
> In MergeAttributes(), in the part that merges the specified column
> definitions into the inherited ones, it says
>
> /*
> * Identity is never inherited. The new column can have an
> * identity definition, so we always just take that one.
> */
> def->identity = newdef->identity;
>
> This is still correct for regular inheritance, but not for partitioning.
> I think for partitioning, this is not reachable because you can't
> specify identity information when you create a partition(?). So maybe
> something like
You may specify the information when creating a partition, but it will
cause an error. We have tests in identity.sql for the same (look for
pitest1_pfail).
>
> if (newdef->identity)
> {
> Assert(!is_partioning);
> /*
> * Identity is never inherited. The new column can have an
> * identity definition, so we always just take that one.
> */
> def->identity = newdef->identity;
> }
>
> Thoughts?
That code block already has Assert(!is_partition) at line 3085. I
thought that Assert is enough.
There's another thing I found. The file isn't using
check_stack_depth() in the function which traverse inheritance
hierarchies. This isn't just a problem of the identity related
function but most of the functions in that file. Do you think it's
worth fixing it?
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-01-22 18:59 Peter Eisentraut <[email protected]>
parent: Ashutosh Bapat <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Peter Eisentraut @ 2024-01-22 18:59 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On 22.01.24 13:23, Ashutosh Bapat wrote:
>> if (newdef->identity)
>> {
>> Assert(!is_partioning);
>> /*
>> * Identity is never inherited. The new column can have an
>> * identity definition, so we always just take that one.
>> */
>> def->identity = newdef->identity;
>> }
>>
>> Thoughts?
>
> That code block already has Assert(!is_partition) at line 3085. I
> thought that Assert is enough.
Ok. Maybe just rephrase that comment somehow then?
> There's another thing I found. The file isn't using
> check_stack_depth() in the function which traverse inheritance
> hierarchies. This isn't just a problem of the identity related
> function but most of the functions in that file. Do you think it's
> worth fixing it?
I suppose the number of inheritance levels is usually not a problem for
stack depth?
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-01-24 06:34 Ashutosh Bapat <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 2 replies; 31+ messages in thread
From: Ashutosh Bapat @ 2024-01-24 06:34 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Tue, Jan 23, 2024 at 12:29 AM Peter Eisentraut <[email protected]> wrote:
>
> On 22.01.24 13:23, Ashutosh Bapat wrote:
> >> if (newdef->identity)
> >> {
> >> Assert(!is_partioning);
> >> /*
> >> * Identity is never inherited. The new column can have an
> >> * identity definition, so we always just take that one.
> >> */
> >> def->identity = newdef->identity;
> >> }
> >>
> >> Thoughts?
> >
> > That code block already has Assert(!is_partition) at line 3085. I
> > thought that Assert is enough.
>
> Ok. Maybe just rephrase that comment somehow then?
Please see refactoring patches attached to [1]. Refactoring that way
makes it unnecessary to mention "regular inheritance" in each comment.
Yet I have included a modified version of the comment in that patch
set.
>
> > There's another thing I found. The file isn't using
> > check_stack_depth() in the function which traverse inheritance
> > hierarchies. This isn't just a problem of the identity related
> > function but most of the functions in that file. Do you think it's
> > worth fixing it?
>
> I suppose the number of inheritance levels is usually not a problem for
> stack depth?
>
Practically it should not. I would rethink the application design if
it requires so many inheritance or partition levels. But functions in
optimizer like try_partitionwise_join() and set_append_rel_size() call
/* Guard against stack overflow due to overly deep inheritance tree. */
check_stack_depth();
I am fine if we want to skip this.
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-01-24 07:02 Ashutosh Bapat <[email protected]>
parent: Ashutosh Bapat <[email protected]>
1 sibling, 0 replies; 31+ messages in thread
From: Ashutosh Bapat @ 2024-01-24 07:02 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Wed, Jan 24, 2024 at 12:04 PM Ashutosh Bapat
<[email protected]> wrote:
> >
> > Ok. Maybe just rephrase that comment somehow then?
>
> Please see refactoring patches attached to [1]. Refactoring that way
> makes it unnecessary to mention "regular inheritance" in each comment.
> Yet I have included a modified version of the comment in that patch
> set.
Sorry forgot to add the reference. Here it is.
[1] https://www.postgresql.org/message-id/[email protected]...
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-02-15 18:00 Alexander Lakhin <[email protected]>
parent: Ashutosh Bapat <[email protected]>
1 sibling, 1 reply; 31+ messages in thread
From: Alexander Lakhin @ 2024-02-15 18:00 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; Peter Eisentraut <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
Hello Ashutosh,
24.01.2024 09:34, Ashutosh Bapat wrote:
>
>>> There's another thing I found. The file isn't using
>>> check_stack_depth() in the function which traverse inheritance
>>> hierarchies. This isn't just a problem of the identity related
>>> function but most of the functions in that file. Do you think it's
>>> worth fixing it?
>> I suppose the number of inheritance levels is usually not a problem for
>> stack depth?
>>
> Practically it should not. I would rethink the application design if
> it requires so many inheritance or partition levels. But functions in
> optimizer like try_partitionwise_join() and set_append_rel_size() call
>
> /* Guard against stack overflow due to overly deep inheritance tree. */
> check_stack_depth();
>
> I am fine if we want to skip this.
I've managed to reach stack overflow inside ATExecSetIdentity() with
the following script:
(echo "CREATE TABLE tp0 (a int PRIMARY KEY,
b int GENERATED ALWAYS AS IDENTITY) PARTITION BY RANGE (a);";
for ((i=1;i<=80000;i++)); do
echo "CREATE TABLE tp$i PARTITION OF tp$(( $i - 1 ))
FOR VALUES FROM ($i) TO (1000000) PARTITION BY RANGE (a);";
done;
echo "ALTER TABLE tp0 ALTER COLUMN b SET GENERATED BY DEFAULT;") | psql >psql.log
(with max_locks_per_transaction = 400 in the config)
It runs about 15 minutes for me and ends with:
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x000055a8ced20de9 in LWLockAcquire (lock=0x7faec200b900, mode=mode@entry=LW_EXCLUSIVE) at lwlock.c:1169
1169 {
(gdb) bt
#0 0x000055a8ced20de9 in LWLockAcquire (lock=0x7faec200b900, mode=mode@entry=LW_EXCLUSIVE) at lwlock.c:1169
#1 0x000055a8cea0342d in WALInsertLockAcquire () at xlog.c:1389
#2 XLogInsertRecord (rdata=0x55a8cf1ccee8 <hdr_rdt>, fpw_lsn=fpw_lsn@entry=1261347512, flags=0 '\000',
num_fpi=num_fpi@entry=0, topxid_included=false) at xlog.c:817
#3 0x000055a8cea1396e in XLogInsert (rmid=rmid@entry=11 '\v', info=<optimized out>) at xloginsert.c:524
#4 0x000055a8ce9c1541 in _bt_insertonpg (rel=0x7faeb8478c98, heaprel=0x7faecf63d378,
itup_key=itup_key@entry=0x55a8d5064678, buf=3210, cbuf=cbuf@entry=0, stack=stack@entry=0x55a8d1063d08,
itup=0x55a8d5064658, itemsz=16,
newitemoff=<optimized out>, postingoff=0, split_only_page=<optimized out>) at nbtinsert.c:1389
#5 0x000055a8ce9bf9a7 in _bt_doinsert (rel=<optimized out>, rel@entry=0x7faeb8478c98, itup=<optimized out>,
itup@entry=0x55a8d5064658, checkUnique=<optimized out>, checkUnique@entry=UNIQUE_CHECK_YES, indexUnchanged=<optimized out>,
heapRel=<optimized out>, heapRel@entry=0x7faecf63d378) at nbtinsert.c:260
#6 0x000055a8ce9c92ad in btinsert (rel=0x7faeb8478c98, values=<optimized out>, isnull=<optimized out>,
ht_ctid=0x55a8d50643cc, heapRel=0x7faecf63d378, checkUnique=UNIQUE_CHECK_YES, indexUnchanged=<optimized out>,
indexInfo=<optimized out>) at nbtree.c:205
#7 0x000055a8cea41391 in CatalogIndexInsert (indstate=indstate@entry=0x55a8d0fc03e8, heapTuple=<optimized out>,
heapTuple@entry=0x55a8d50643c8, updateIndexes=<optimized out>) at indexing.c:170
#8 0x000055a8cea4172c in CatalogTupleUpdate (heapRel=heapRel@entry=0x7faecf63d378, otid=0x55a8d50643cc,
tup=tup@entry=0x55a8d50643c8) at indexing.c:324
#9 0x000055a8ceb18173 in ATExecSetIdentity (rel=0x7faeab1288a8, colName=colName@entry=0x55a8d0fbc2b8 "b",
def=def@entry=0x55a8d1063918, lockmode=lockmode@entry=8, recurse=true, recursing=<optimized out>) at tablecmds.c:8307
#10 0x000055a8ceb18251 in ATExecSetIdentity (rel=0x7faeab127f28, colName=colName@entry=0x55a8d0fbc2b8 "b",
def=def@entry=0x55a8d1063918, lockmode=lockmode@entry=8, recurse=true, recursing=<optimized out>) at tablecmds.c:8337
#11 0x000055a8ceb18251 in ATExecSetIdentity (rel=0x7faeab1275a8, colName=colName@entry=0x55a8d0fbc2b8 "b",
def=def@entry=0x55a8d1063918, lockmode=lockmode@entry=8, recurse=true, recursing=<optimized out>) at tablecmds.c:8337
#12 0x000055a8ceb18251 in ATExecSetIdentity (rel=0x7faeab126c28, colName=colName@entry=0x55a8d0fbc2b8 "b",
def=def@entry=0x55a8d1063918, lockmode=lockmode@entry=8, recurse=true, recursing=<optimized out>) at tablecmds.c:8337
...
Functions ATExecAddIdentity() and ATExecDropIdentity() are recursive too,
so I think they can be exploited as well.
Best regards,
Alexander
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-02-19 12:17 Ashutosh Bapat <[email protected]>
parent: Alexander Lakhin <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Ashutosh Bapat @ 2024-02-19 12:17 UTC (permalink / raw)
To: Alexander Lakhin <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Feb 15, 2024 at 11:30 PM Alexander Lakhin <[email protected]> wrote:
>
> Hello Ashutosh,
>
> 24.01.2024 09:34, Ashutosh Bapat wrote:
> >
> >>> There's another thing I found. The file isn't using
> >>> check_stack_depth() in the function which traverse inheritance
> >>> hierarchies. This isn't just a problem of the identity related
> >>> function but most of the functions in that file. Do you think it's
> >>> worth fixing it?
> >> I suppose the number of inheritance levels is usually not a problem for
> >> stack depth?
> >>
> > Practically it should not. I would rethink the application design if
> > it requires so many inheritance or partition levels. But functions in
> > optimizer like try_partitionwise_join() and set_append_rel_size() call
> >
> > /* Guard against stack overflow due to overly deep inheritance tree. */
> > check_stack_depth();
> >
> > I am fine if we want to skip this.
>
> I've managed to reach stack overflow inside ATExecSetIdentity() with
> the following script:
> (echo "CREATE TABLE tp0 (a int PRIMARY KEY,
> b int GENERATED ALWAYS AS IDENTITY) PARTITION BY RANGE (a);";
> for ((i=1;i<=80000;i++)); do
> echo "CREATE TABLE tp$i PARTITION OF tp$(( $i - 1 ))
> FOR VALUES FROM ($i) TO (1000000) PARTITION BY RANGE (a);";
> done;
> echo "ALTER TABLE tp0 ALTER COLUMN b SET GENERATED BY DEFAULT;") | psql >psql.log
>
> (with max_locks_per_transaction = 400 in the config)
>
> It runs about 15 minutes for me and ends with:
> Program terminated with signal SIGSEGV, Segmentation fault.
>
> #0 0x000055a8ced20de9 in LWLockAcquire (lock=0x7faec200b900, mode=mode@entry=LW_EXCLUSIVE) at lwlock.c:1169
> 1169 {
> (gdb) bt
> #0 0x000055a8ced20de9 in LWLockAcquire (lock=0x7faec200b900, mode=mode@entry=LW_EXCLUSIVE) at lwlock.c:1169
> #1 0x000055a8cea0342d in WALInsertLockAcquire () at xlog.c:1389
> #2 XLogInsertRecord (rdata=0x55a8cf1ccee8 <hdr_rdt>, fpw_lsn=fpw_lsn@entry=1261347512, flags=0 '\000',
> num_fpi=num_fpi@entry=0, topxid_included=false) at xlog.c:817
> #3 0x000055a8cea1396e in XLogInsert (rmid=rmid@entry=11 '\v', info=<optimized out>) at xloginsert.c:524
> #4 0x000055a8ce9c1541 in _bt_insertonpg (rel=0x7faeb8478c98, heaprel=0x7faecf63d378,
> itup_key=itup_key@entry=0x55a8d5064678, buf=3210, cbuf=cbuf@entry=0, stack=stack@entry=0x55a8d1063d08,
> itup=0x55a8d5064658, itemsz=16,
> newitemoff=<optimized out>, postingoff=0, split_only_page=<optimized out>) at nbtinsert.c:1389
> #5 0x000055a8ce9bf9a7 in _bt_doinsert (rel=<optimized out>, rel@entry=0x7faeb8478c98, itup=<optimized out>,
> itup@entry=0x55a8d5064658, checkUnique=<optimized out>, checkUnique@entry=UNIQUE_CHECK_YES, indexUnchanged=<optimized out>,
> heapRel=<optimized out>, heapRel@entry=0x7faecf63d378) at nbtinsert.c:260
> #6 0x000055a8ce9c92ad in btinsert (rel=0x7faeb8478c98, values=<optimized out>, isnull=<optimized out>,
> ht_ctid=0x55a8d50643cc, heapRel=0x7faecf63d378, checkUnique=UNIQUE_CHECK_YES, indexUnchanged=<optimized out>,
> indexInfo=<optimized out>) at nbtree.c:205
> #7 0x000055a8cea41391 in CatalogIndexInsert (indstate=indstate@entry=0x55a8d0fc03e8, heapTuple=<optimized out>,
> heapTuple@entry=0x55a8d50643c8, updateIndexes=<optimized out>) at indexing.c:170
> #8 0x000055a8cea4172c in CatalogTupleUpdate (heapRel=heapRel@entry=0x7faecf63d378, otid=0x55a8d50643cc,
> tup=tup@entry=0x55a8d50643c8) at indexing.c:324
> #9 0x000055a8ceb18173 in ATExecSetIdentity (rel=0x7faeab1288a8, colName=colName@entry=0x55a8d0fbc2b8 "b",
> def=def@entry=0x55a8d1063918, lockmode=lockmode@entry=8, recurse=true, recursing=<optimized out>) at tablecmds.c:8307
> #10 0x000055a8ceb18251 in ATExecSetIdentity (rel=0x7faeab127f28, colName=colName@entry=0x55a8d0fbc2b8 "b",
> def=def@entry=0x55a8d1063918, lockmode=lockmode@entry=8, recurse=true, recursing=<optimized out>) at tablecmds.c:8337
> #11 0x000055a8ceb18251 in ATExecSetIdentity (rel=0x7faeab1275a8, colName=colName@entry=0x55a8d0fbc2b8 "b",
> def=def@entry=0x55a8d1063918, lockmode=lockmode@entry=8, recurse=true, recursing=<optimized out>) at tablecmds.c:8337
> #12 0x000055a8ceb18251 in ATExecSetIdentity (rel=0x7faeab126c28, colName=colName@entry=0x55a8d0fbc2b8 "b",
> def=def@entry=0x55a8d1063918, lockmode=lockmode@entry=8, recurse=true, recursing=<optimized out>) at tablecmds.c:8337
> ...
>
> Functions ATExecAddIdentity() and ATExecDropIdentity() are recursive too,
> so I think they can be exploited as well.
not just Identity related functions, but many other functions in
tablecmds.c have that problem as I mentioned earlier.
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-02-19 15:00 Alexander Lakhin <[email protected]>
parent: Ashutosh Bapat <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Alexander Lakhin @ 2024-02-19 15:00 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>
Hello Ashutosh,
19.02.2024 15:17, Ashutosh Bapat wrote:
>
>> Functions ATExecAddIdentity() and ATExecDropIdentity() are recursive too,
>> so I think they can be exploited as well.
> not just Identity related functions, but many other functions in
> tablecmds.c have that problem as I mentioned earlier.
>
Could you please name functions, which you suspect, for me to recheck them?
Perhaps we should consider fixing all of such functions, in light of
b0f7dd915 and d57b7cc33...
Best regards,
Alexander
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-02-20 04:57 Ashutosh Bapat <[email protected]>
parent: Alexander Lakhin <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Ashutosh Bapat @ 2024-02-20 04:57 UTC (permalink / raw)
To: Alexander Lakhin <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>
On Mon, Feb 19, 2024 at 8:30 PM Alexander Lakhin <[email protected]> wrote:
>
> Hello Ashutosh,
>
> 19.02.2024 15:17, Ashutosh Bapat wrote:
> >
> >> Functions ATExecAddIdentity() and ATExecDropIdentity() are recursive too,
> >> so I think they can be exploited as well.
> > not just Identity related functions, but many other functions in
> > tablecmds.c have that problem as I mentioned earlier.
> >
>
> Could you please name functions, which you suspect, for me to recheck them?
> Perhaps we should consider fixing all of such functions, in light of
> b0f7dd915 and d57b7cc33...
Looks like the second commit has fixed all other places I knew except
Identity related functions. So worth fixing identity related functions
too. I see
dropconstraint_internal() has two calls to check_stack_depth() back to
back. The second one is not needed?
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-02-20 06:00 Alexander Lakhin <[email protected]>
parent: Ashutosh Bapat <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Alexander Lakhin @ 2024-02-20 06:00 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>
20.02.2024 07:57, Ashutosh Bapat wrote:
>> Could you please name functions, which you suspect, for me to recheck them?
>> Perhaps we should consider fixing all of such functions, in light of
>> b0f7dd915 and d57b7cc33...
> Looks like the second commit has fixed all other places I knew except
> Identity related functions. So worth fixing identity related functions
> too. I see
> dropconstraint_internal() has two calls to check_stack_depth() back to
> back. The second one is not needed?
Yeah, that's funny. It looks like such a double protection emerged
because Alvaro protected the function (in b0f7dd915), which was waiting for
adding check_stack_depth() in the other thread (resulted in d57b7cc33).
Thank you for spending time on this!
Best regards,
Alexander
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-02-21 00:51 Alexander Korotkov <[email protected]>
parent: Alexander Lakhin <[email protected]>
0 siblings, 0 replies; 31+ messages in thread
From: Alexander Korotkov @ 2024-02-21 00:51 UTC (permalink / raw)
To: Alexander Lakhin <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; Alexander Korotkov <[email protected]>; Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Feb 20, 2024 at 8:00 AM Alexander Lakhin <[email protected]> wrote:
> 20.02.2024 07:57, Ashutosh Bapat wrote:
> >> Could you please name functions, which you suspect, for me to recheck them?
> >> Perhaps we should consider fixing all of such functions, in light of
> >> b0f7dd915 and d57b7cc33...
> > Looks like the second commit has fixed all other places I knew except
> > Identity related functions. So worth fixing identity related functions
> > too. I see
> > dropconstraint_internal() has two calls to check_stack_depth() back to
> > back. The second one is not needed?
>
> Yeah, that's funny. It looks like such a double protection emerged
> because Alvaro protected the function (in b0f7dd915), which was waiting for
> adding check_stack_depth() in the other thread (resulted in d57b7cc33).
>
> Thank you for spending time on this!
Thank you, I removed the second check.
------
Regards,
Alexander Korotkov
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-04-26 12:00 Alexander Lakhin <[email protected]>
parent: Peter Eisentraut <[email protected]>
1 sibling, 1 reply; 31+ messages in thread
From: Alexander Lakhin @ 2024-04-26 12:00 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; Ashutosh Bapat <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
Hello Ashutosh and Peter,
16.01.2024 21:59, Peter Eisentraut wrote:
> On 09.01.24 15:10, Ashutosh Bapat wrote:
>> Here's complete patch-set.
>
> Looks good! Committed.
>
Please take a look at a new error case introduced by 699586315:
CREATE TABLE tbl1 (a int PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY)
PARTITION BY LIST (a);
CREATE TABLE tbl2 PARTITION OF tbl1 DEFAULT;
CREATE TABLE tbl3 (LIKE tbl2 INCLUDING IDENTITY);
ERROR: no owned sequence found
Best regards,
Alexander
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-04-26 12:57 Ashutosh Bapat <[email protected]>
parent: Alexander Lakhin <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Ashutosh Bapat @ 2024-04-26 12:57 UTC (permalink / raw)
To: Alexander Lakhin <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>
Thanks Alexander for the report.
On Fri, Apr 26, 2024 at 5:30 PM Alexander Lakhin <[email protected]>
wrote:
> Hello Ashutosh and Peter,
>
> 16.01.2024 21:59, Peter Eisentraut wrote:
> > On 09.01.24 15:10, Ashutosh Bapat wrote:
> >> Here's complete patch-set.
> >
> > Looks good! Committed.
> >
>
> Please take a look at a new error case introduced by 699586315:
> CREATE TABLE tbl1 (a int PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY)
> PARTITION BY LIST (a);
> CREATE TABLE tbl2 PARTITION OF tbl1 DEFAULT;
>
> CREATE TABLE tbl3 (LIKE tbl2 INCLUDING IDENTITY);
> ERROR: no owned sequence found
>
I don't think creating a table like a partition is common or even useful.
Usually it would create it from partitithe oned table. But if we consider
that to be a use case, I think the error is expected since a partition
doesn't have its own identity; it shares it with the partitioned table.
Maybe we could give a better message. But I will look into this and fix it
if the solution makes sense.
Do you want to track this in open items?
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-04-26 18:00 Alexander Lakhin <[email protected]>
parent: Ashutosh Bapat <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Alexander Lakhin @ 2024-04-26 18:00 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>
26.04.2024 15:57, Ashutosh Bapat wrote:
> Thanks Alexander for the report.
>
> On Fri, Apr 26, 2024 at 5:30 PM Alexander Lakhin <[email protected]> wrote:
>
>
> CREATE TABLE tbl3 (LIKE tbl2 INCLUDING IDENTITY);
> ERROR: no owned sequence found
>
>
> I don't think creating a table like a partition is common or even useful. Usually it would create it from partitithe
> oned table. But if we consider that to be a use case, I think the error is expected since a partition doesn't have its
> own identity; it shares it with the partitioned table. Maybe we could give a better message. But I will look into this
> and fix it if the solution makes sense.
Maybe it's uncommon, but it's allowed, so users may want to
CREATE TABLE sometable (LIKE partX INCLUDING ALL), for example, if the
partition has a somewhat different structure. And thinking about how such
a restriction could be described in the docs, I would prefer to avoid this
error at the implementation level.
>
> Do you want to track this in open items?
>
If you are inclined to fix this behavior, I would add this item.
Best regards,
Alexander
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-04-27 15:00 Alexander Lakhin <[email protected]>
parent: Alexander Lakhin <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Alexander Lakhin @ 2024-04-27 15:00 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>
Hello Ashutosh,
26.04.2024 21:00, Alexander Lakhin wrote:
> 26.04.2024 15:57, Ashutosh Bapat wrote:
>> Thanks Alexander for the report.
>>
>> On Fri, Apr 26, 2024 at 5:30 PM Alexander Lakhin <[email protected]> wrote:
>>
>>
>> CREATE TABLE tbl3 (LIKE tbl2 INCLUDING IDENTITY);
>> ERROR: no owned sequence found
>>
>>
>
>>
>> Do you want to track this in open items?
>>
>
> If you are inclined to fix this behavior, I would add this item.
Please look also at another script, which produces the same error:
CREATE TABLE tbl1 (a int GENERATED BY DEFAULT AS IDENTITY, b text)
PARTITION BY LIST (b);
CREATE TABLE tbl2 PARTITION OF tbl1 DEFAULT;
ALTER TABLE tbl1 ALTER COLUMN a SET DATA TYPE bigint;
ERROR: no owned sequence found
(On 699586315~1, it executes successfully and changes the data type of the
identity column and it's sequence.)
Best regards,
Alexander
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-04-28 06:59 Alexander Lakhin <[email protected]>
parent: Alexander Lakhin <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Alexander Lakhin @ 2024-04-28 06:59 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>
27.04.2024 18:00, Alexander Lakhin wrote:
>
> Please look also at another script, which produces the same error:
I've discovered yet another problematic case:
CREATE TABLE tbl1 (a int GENERATED ALWAYS AS IDENTITY, b text)
PARTITION BY LIST (a);
CREATE TABLE tbl2 (b text, a int NOT NULL);
ALTER TABLE tbl1 ATTACH PARTITION tbl2 DEFAULT;
INSERT INTO tbl2 DEFAULT VALUES;
ERROR: no owned sequence found
Though it works with tbl2(a int NOT NULL, b text)...
Take a look at this too, please.
Best regards,
Alexander
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-04-30 10:59 Ashutosh Bapat <[email protected]>
parent: Alexander Lakhin <[email protected]>
0 siblings, 3 replies; 31+ messages in thread
From: Ashutosh Bapat @ 2024-04-30 10:59 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; Alexander Lakhin <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Sun, Apr 28, 2024 at 12:29 PM Alexander Lakhin <[email protected]>
wrote:
> 27.04.2024 18:00, Alexander Lakhin wrote:
> >
> > Please look also at another script, which produces the same error:
>
> I've discovered yet another problematic case:
> CREATE TABLE tbl1 (a int GENERATED ALWAYS AS IDENTITY, b text)
> PARTITION BY LIST (a);
> CREATE TABLE tbl2 (b text, a int NOT NULL);
> ALTER TABLE tbl1 ATTACH PARTITION tbl2 DEFAULT;
>
> INSERT INTO tbl2 DEFAULT VALUES;
> ERROR: no owned sequence found
>
> Though it works with tbl2(a int NOT NULL, b text)...
> Take a look at this too, please.
>
Thanks Alexander for the report.
PFA patch which fixes all the three problems.
I had not fixed getIdentitySequence() to fetch identity sequence associated
with the partition because I thought it would be better to fail with an
error when it's not used correctly. But these bugs show 1. the error is
misleading and unwanted 2. there are more places where adding that logic
to getIdentitySequence() makes sense. Fixed the function in these patches.
Now callers like transformAlterTableStmt have to be careful not to call the
function on a partition.
I have examined all the callers of getIdentitySequence() and they seem to
be fine. The code related to SetIdentity, DropIdentity is not called for
partitions, errors for which are thrown elsewhere earlier.
--
Best Wishes,
Ashutosh Bapat
Attachments:
[text/x-patch] 0001-Fix-assorted-bugs-related-to-identity-colum-20240430.patch (13.5K, ../../CAExHW5vK-Pvrupw30iK9_10z18oRTYevpGJhv6hLupW6no-CJA@mail.gmail.com/3-0001-Fix-assorted-bugs-related-to-identity-colum-20240430.patch)
download | inline diff:
From bce2e8d573040901b18c0c9f6884f0fd44f50724 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email protected]>
Date: Tue, 30 Apr 2024 15:32:34 +0530
Subject: [PATCH] Fix assorted bugs related to identity column in partitioned
tables
When changing data type of a column of a partitioned table craft ALTER SEQUENCE
command only once. Partitions do not have identity sequences of their own and
thus do not need ALTER SEQUENCE command for each partition.
Fix getIdentitySequence() to fetch the identity sequence associated with the
top level partitioned table when Relation of a partition is passed to it. While
doing so translate the attribute number of the partition into the attribute
number of the partitioned table.
Author: Ashutosh Bapat
Discussion: https://www.postgresql.org/message-id/[email protected]
---
src/backend/catalog/pg_depend.c | 31 ++++++++++++++-
src/backend/commands/tablecmds.c | 2 +-
src/backend/parser/parse_utilcmd.c | 52 +++++++++++++++-----------
src/backend/rewrite/rewriteHandler.c | 19 +---------
src/include/catalog/dependency.h | 2 +-
src/test/regress/expected/identity.out | 32 +++++++++++++++-
src/test/regress/sql/identity.sql | 11 +++++-
7 files changed, 101 insertions(+), 48 deletions(-)
diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index f85a898de8..5366f7820c 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -23,10 +23,12 @@
#include "catalog/pg_constraint.h"
#include "catalog/pg_depend.h"
#include "catalog/pg_extension.h"
+#include "catalog/partition.h"
#include "commands/extension.h"
#include "miscadmin.h"
#include "utils/fmgroids.h"
#include "utils/lsyscache.h"
+#include "utils/syscache.h"
#include "utils/rel.h"
@@ -941,10 +943,35 @@ getOwnedSequences(Oid relid)
* Get owned identity sequence, error if not exactly one.
*/
Oid
-getIdentitySequence(Oid relid, AttrNumber attnum, bool missing_ok)
+getIdentitySequence(Relation rel, AttrNumber attnum, bool missing_ok)
{
- List *seqlist = getOwnedSequences_internal(relid, attnum, DEPENDENCY_INTERNAL);
+ Oid relid;
+ List *seqlist;
+ /*
+ * The identity sequence is associated with the topmost partitioned table,
+ * which might have column order different than the given partition.
+ */
+ if (RelationGetForm(rel)->relispartition)
+ {
+ List *ancestors =
+ get_partition_ancestors(RelationGetRelid(rel));
+ HeapTuple ctup = SearchSysCacheAttNum(RelationGetRelid(rel), attnum);
+ const char *attname = NameStr(((Form_pg_attribute) GETSTRUCT(ctup))->attname);
+ HeapTuple ptup;
+
+ relid = llast_oid(ancestors);
+ ptup = SearchSysCacheAttName(relid, attname);
+ attnum = ((Form_pg_attribute) GETSTRUCT(ptup))->attnum;
+
+ ReleaseSysCache(ctup);
+ ReleaseSysCache(ptup);
+ list_free(ancestors);
+ }
+ else
+ relid = RelationGetRelid(rel);
+
+ seqlist = getOwnedSequences_internal(relid, attnum, DEPENDENCY_INTERNAL);
if (list_length(seqlist) > 1)
elog(ERROR, "more than one owned sequence found");
else if (seqlist == NIL)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3556240c8e..9d32b2c495 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8516,7 +8516,7 @@ ATExecDropIdentity(Relation rel, const char *colName, bool missing_ok, LOCKMODE
if (!recursing)
{
/* drop the internal sequence */
- seqid = getIdentitySequence(RelationGetRelid(rel), attnum, false);
+ seqid = getIdentitySequence(rel, attnum, false);
deleteDependencyRecordsForClass(RelationRelationId, seqid,
RelationRelationId, DEPENDENCY_INTERNAL);
CommandCounterIncrement();
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index fef084f5d5..5e6bd58c3a 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -1126,7 +1126,7 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla
* find sequence owned by old column; extract sequence parameters;
* build new create sequence command
*/
- seq_relid = getIdentitySequence(RelationGetRelid(relation), attribute->attnum, false);
+ seq_relid = getIdentitySequence(relation, attribute->attnum, false);
seq_options = sequence_options(seq_relid);
generateSerialExtraStmts(cxt, def,
InvalidOid, seq_options,
@@ -3706,28 +3706,36 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
/*
* For identity column, create ALTER SEQUENCE command to
- * change the data type of the sequence.
+ * change the data type of the sequence. Identity sequence
+ * is associated with the top level partitioned table.
+ * Hence ignore partitions.
*/
- attnum = get_attnum(relid, cmd->name);
- if (attnum == InvalidAttrNumber)
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_COLUMN),
- errmsg("column \"%s\" of relation \"%s\" does not exist",
- cmd->name, RelationGetRelationName(rel))));
-
- if (attnum > 0 &&
- TupleDescAttr(tupdesc, attnum - 1)->attidentity)
+ if (!RelationGetForm(rel)->relispartition)
{
- Oid seq_relid = getIdentitySequence(relid, attnum, false);
- Oid typeOid = typenameTypeId(pstate, def->typeName);
- AlterSeqStmt *altseqstmt = makeNode(AlterSeqStmt);
-
- altseqstmt->sequence = makeRangeVar(get_namespace_name(get_rel_namespace(seq_relid)),
- get_rel_name(seq_relid),
- -1);
- altseqstmt->options = list_make1(makeDefElem("as", (Node *) makeTypeNameFromOid(typeOid, -1), -1));
- altseqstmt->for_identity = true;
- cxt.blist = lappend(cxt.blist, altseqstmt);
+ attnum = get_attnum(relid, cmd->name);
+ if (attnum == InvalidAttrNumber)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg("column \"%s\" of relation \"%s\" does not exist",
+ cmd->name, RelationGetRelationName(rel))));
+
+ if (attnum > 0 &&
+ TupleDescAttr(tupdesc, attnum - 1)->attidentity)
+ {
+ Oid seq_relid = getIdentitySequence(rel, attnum, false);
+ Oid typeOid = typenameTypeId(pstate, def->typeName);
+ AlterSeqStmt *altseqstmt = makeNode(AlterSeqStmt);
+
+ altseqstmt->sequence
+ = makeRangeVar(get_namespace_name(get_rel_namespace(seq_relid)),
+ get_rel_name(seq_relid),
+ -1);
+ altseqstmt->options = list_make1(makeDefElem("as",
+ (Node *) makeTypeNameFromOid(typeOid, -1),
+ -1));
+ altseqstmt->for_identity = true;
+ cxt.blist = lappend(cxt.blist, altseqstmt);
+ }
}
newcmds = lappend(newcmds, cmd);
@@ -3793,7 +3801,7 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
errmsg("column \"%s\" of relation \"%s\" does not exist",
cmd->name, RelationGetRelationName(rel))));
- seq_relid = getIdentitySequence(relid, attnum, true);
+ seq_relid = getIdentitySequence(rel, attnum, true);
if (seq_relid)
{
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 9fd05b15e7..8a29fbbc46 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -24,7 +24,6 @@
#include "access/sysattr.h"
#include "access/table.h"
#include "catalog/dependency.h"
-#include "catalog/partition.h"
#include "commands/trigger.h"
#include "executor/executor.h"
#include "foreign/fdwapi.h"
@@ -1233,24 +1232,8 @@ build_column_default(Relation rel, int attrno)
if (att_tup->attidentity)
{
NextValueExpr *nve = makeNode(NextValueExpr);
- Oid reloid;
- /*
- * The identity sequence is associated with the topmost partitioned
- * table.
- */
- if (rel->rd_rel->relispartition)
- {
- List *ancestors =
- get_partition_ancestors(RelationGetRelid(rel));
-
- reloid = llast_oid(ancestors);
- list_free(ancestors);
- }
- else
- reloid = RelationGetRelid(rel);
-
- nve->seqid = getIdentitySequence(reloid, attrno, false);
+ nve->seqid = getIdentitySequence(rel, attrno, false);
nve->typeId = att_tup->atttypid;
return (Node *) nve;
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index ec654010d4..f861d5c8a5 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -170,7 +170,7 @@ extern List *getAutoExtensionsOfObject(Oid classId, Oid objectId);
extern bool sequenceIsOwned(Oid seqId, char deptype, Oid *tableId, int32 *colId);
extern List *getOwnedSequences(Oid relid);
-extern Oid getIdentitySequence(Oid relid, AttrNumber attnum, bool missing_ok);
+extern Oid getIdentitySequence(Relation rel, AttrNumber attnum, bool missing_ok);
extern Oid get_index_constraint(Oid indexId);
diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out
index f357b9b63b..4f67e75918 100644
--- a/src/test/regress/expected/identity.out
+++ b/src/test/regress/expected/identity.out
@@ -619,14 +619,17 @@ CREATE TABLE pitest1_p1 PARTITION OF pitest1 FOR VALUES FROM ('2016-07-01') TO (
INSERT into pitest1(f1, f2) VALUES ('2016-07-2', 'from pitest1');
INSERT into pitest1_p1 (f1, f2) VALUES ('2016-07-3', 'from pitest1_p1');
-- attached partition
-CREATE TABLE pitest1_p2 (f1 date NOT NULL, f2 text, f3 bigint);
-INSERT INTO pitest1_p2 VALUES ('2016-08-2', 'before attaching', 100);
+CREATE TABLE pitest1_p2 (f3 bigint, f2 text, f1 date NOT NULL);
+INSERT INTO pitest1_p2 (f1, f2, f3) VALUES ('2016-08-2', 'before attaching', 100);
ALTER TABLE pitest1 ATTACH PARTITION pitest1_p2 FOR VALUES FROM ('2016-08-01') TO ('2016-09-01'); -- requires NOT NULL constraint
ERROR: column "f3" in child table must be marked NOT NULL
ALTER TABLE pitest1_p2 ALTER COLUMN f3 SET NOT NULL;
ALTER TABLE pitest1 ATTACH PARTITION pitest1_p2 FOR VALUES FROM ('2016-08-01') TO ('2016-09-01');
INSERT INTO pitest1_p2 (f1, f2) VALUES ('2016-08-3', 'from pitest1_p2');
INSERT INTO pitest1 (f1, f2) VALUES ('2016-08-4', 'from pitest1');
+-- LIKE INCLUDING on partition
+CREATE TABLE pitest1_p1_like(LIKE pitest1_p1 INCLUDING IDENTITY);
+INSERT into pitest1_p1_like(f1, f2) VALUES ('2016-07-2', 'from pitest1_p1_like');
SELECT tableoid::regclass, f1, f2, f3 FROM pitest1;
tableoid | f1 | f2 | f3
------------+------------+------------------+-----
@@ -637,6 +640,31 @@ SELECT tableoid::regclass, f1, f2, f3 FROM pitest1;
pitest1_p2 | 08-04-2016 | from pitest1 | 4
(5 rows)
+SELECT tableoid::regclass, f1, f2, f3 FROM pitest1_p1_like;
+ tableoid | f1 | f2 | f3
+-----------------+------------+----------------------+----
+ pitest1_p1_like | 07-02-2016 | from pitest1_p1_like | 1
+(1 row)
+
+ALTER TABLE pitest1 ALTER COLUMN f3 SET DATA TYPE bigint;
+SELECT tableoid::regclass, f1, f2, f3, pg_typeof(f3) FROM pitest1;
+ tableoid | f1 | f2 | f3 | pg_typeof
+------------+------------+------------------+-----+-----------
+ pitest1_p1 | 07-02-2016 | from pitest1 | 1 | bigint
+ pitest1_p1 | 07-03-2016 | from pitest1_p1 | 2 | bigint
+ pitest1_p2 | 08-02-2016 | before attaching | 100 | bigint
+ pitest1_p2 | 08-03-2016 | from pitest1_p2 | 3 | bigint
+ pitest1_p2 | 08-04-2016 | from pitest1 | 4 | bigint
+(5 rows)
+
+SELECT tableoid::regclass, f1, f2, f3, pg_typeof(f3) FROM pitest1_p2;
+ tableoid | f1 | f2 | f3 | pg_typeof
+------------+------------+------------------+-----+-----------
+ pitest1_p2 | 08-02-2016 | before attaching | 100 | bigint
+ pitest1_p2 | 08-03-2016 | from pitest1_p2 | 3 | bigint
+ pitest1_p2 | 08-04-2016 | from pitest1 | 4 | bigint
+(3 rows)
+
-- add identity column
CREATE TABLE pitest2 (f1 date NOT NULL, f2 text) PARTITION BY RANGE (f1);
CREATE TABLE pitest2_p1 PARTITION OF pitest2 FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
diff --git a/src/test/regress/sql/identity.sql b/src/test/regress/sql/identity.sql
index 7b0800226c..c921074058 100644
--- a/src/test/regress/sql/identity.sql
+++ b/src/test/regress/sql/identity.sql
@@ -358,14 +358,21 @@ CREATE TABLE pitest1_p1 PARTITION OF pitest1 FOR VALUES FROM ('2016-07-01') TO (
INSERT into pitest1(f1, f2) VALUES ('2016-07-2', 'from pitest1');
INSERT into pitest1_p1 (f1, f2) VALUES ('2016-07-3', 'from pitest1_p1');
-- attached partition
-CREATE TABLE pitest1_p2 (f1 date NOT NULL, f2 text, f3 bigint);
-INSERT INTO pitest1_p2 VALUES ('2016-08-2', 'before attaching', 100);
+CREATE TABLE pitest1_p2 (f3 bigint, f2 text, f1 date NOT NULL);
+INSERT INTO pitest1_p2 (f1, f2, f3) VALUES ('2016-08-2', 'before attaching', 100);
ALTER TABLE pitest1 ATTACH PARTITION pitest1_p2 FOR VALUES FROM ('2016-08-01') TO ('2016-09-01'); -- requires NOT NULL constraint
ALTER TABLE pitest1_p2 ALTER COLUMN f3 SET NOT NULL;
ALTER TABLE pitest1 ATTACH PARTITION pitest1_p2 FOR VALUES FROM ('2016-08-01') TO ('2016-09-01');
INSERT INTO pitest1_p2 (f1, f2) VALUES ('2016-08-3', 'from pitest1_p2');
INSERT INTO pitest1 (f1, f2) VALUES ('2016-08-4', 'from pitest1');
+-- LIKE INCLUDING on partition
+CREATE TABLE pitest1_p1_like(LIKE pitest1_p1 INCLUDING IDENTITY);
+INSERT into pitest1_p1_like(f1, f2) VALUES ('2016-07-2', 'from pitest1_p1_like');
SELECT tableoid::regclass, f1, f2, f3 FROM pitest1;
+SELECT tableoid::regclass, f1, f2, f3 FROM pitest1_p1_like;
+ALTER TABLE pitest1 ALTER COLUMN f3 SET DATA TYPE bigint;
+SELECT tableoid::regclass, f1, f2, f3, pg_typeof(f3) FROM pitest1;
+SELECT tableoid::regclass, f1, f2, f3, pg_typeof(f3) FROM pitest1_p2;
-- add identity column
CREATE TABLE pitest2 (f1 date NOT NULL, f2 text) PARTITION BY RANGE (f1);
--
2.34.1
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-05-01 03:53 Michael Paquier <[email protected]>
parent: Ashutosh Bapat <[email protected]>
2 siblings, 0 replies; 31+ messages in thread
From: Michael Paquier @ 2024-05-01 03:53 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Alexander Lakhin <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Apr 30, 2024 at 04:29:11PM +0530, Ashutosh Bapat wrote:
> PFA patch which fixes all the three problems.
Please note that this was not tracked as an open item, so I have added
one referring to the failures reported by Alexander.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-05-04 20:13 Dmitry Dolgov <[email protected]>
parent: Ashutosh Bapat <[email protected]>
2 siblings, 1 reply; 31+ messages in thread
From: Dmitry Dolgov @ 2024-05-04 20:13 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Alexander Lakhin <[email protected]>; PostgreSQL Hackers <[email protected]>
> On Tue, Apr 30, 2024 at 04:29:11PM +0530, Ashutosh Bapat wrote:
> On Sun, Apr 28, 2024 at 12:29 PM Alexander Lakhin <[email protected]>
> wrote:
>
> > 27.04.2024 18:00, Alexander Lakhin wrote:
> > >
> > > Please look also at another script, which produces the same error:
> >
> > I've discovered yet another problematic case:
> > CREATE TABLE tbl1 (a int GENERATED ALWAYS AS IDENTITY, b text)
> > PARTITION BY LIST (a);
> > CREATE TABLE tbl2 (b text, a int NOT NULL);
> > ALTER TABLE tbl1 ATTACH PARTITION tbl2 DEFAULT;
> >
> > INSERT INTO tbl2 DEFAULT VALUES;
> > ERROR: no owned sequence found
> >
> > Though it works with tbl2(a int NOT NULL, b text)...
> > Take a look at this too, please.
> >
>
> Thanks Alexander for the report.
>
> PFA patch which fixes all the three problems.
>
> I had not fixed getIdentitySequence() to fetch identity sequence associated
> with the partition because I thought it would be better to fail with an
> error when it's not used correctly. But these bugs show 1. the error is
> misleading and unwanted 2. there are more places where adding that logic
> to getIdentitySequence() makes sense. Fixed the function in these patches.
> Now callers like transformAlterTableStmt have to be careful not to call the
> function on a partition.
>
> I have examined all the callers of getIdentitySequence() and they seem to
> be fine. The code related to SetIdentity, DropIdentity is not called for
> partitions, errors for which are thrown elsewhere earlier.
Thanks for the fix.
I had a quick look, it covers the issues mentioned above in the thread.
Few nitpicks/questions:
* I think it makes sense to verify if the ptup is valid. This approach
would fail if the target column of the root partition is marked as
attisdropped.
Oid
-getIdentitySequence(Oid relid, AttrNumber attnum, bool missing_ok)
+getIdentitySequence(Relation rel, AttrNumber attnum, bool missing_ok)
{
[...]
+ relid = llast_oid(ancestors);
+ ptup = SearchSysCacheAttName(relid, attname);
+ attnum = ((Form_pg_attribute) GETSTRUCT(ptup))->attnum;
* getIdentitySequence is used in build_column_default, which in turn
often appears in loops over table attributes. AFAICT it means that the
same root partition search will be repeated multiple times in such
situations if there is more than one identity. I assume the
performance impact of this repetition is negligible?
* Maybe a silly question, but since I'm not aware about all the details
here, I'm curious -- the approach of mapping attributes of a partition
to the root partition attributes, how robust is it? I guess there is
no way that the root partition column will be not what is expected,
e.g. due to some sort of concurrency?
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-05-06 13:22 Ashutosh Bapat <[email protected]>
parent: Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 31+ messages in thread
From: Ashutosh Bapat @ 2024-05-06 13:22 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Alexander Lakhin <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sun, May 5, 2024 at 1:43 AM Dmitry Dolgov <[email protected]> wrote:
> > On Tue, Apr 30, 2024 at 04:29:11PM +0530, Ashutosh Bapat wrote:
> > On Sun, Apr 28, 2024 at 12:29 PM Alexander Lakhin <[email protected]>
> > wrote:
> >
> > > 27.04.2024 18:00, Alexander Lakhin wrote:
> > > >
> > > > Please look also at another script, which produces the same error:
> > >
> > > I've discovered yet another problematic case:
> > > CREATE TABLE tbl1 (a int GENERATED ALWAYS AS IDENTITY, b text)
> > > PARTITION BY LIST (a);
> > > CREATE TABLE tbl2 (b text, a int NOT NULL);
> > > ALTER TABLE tbl1 ATTACH PARTITION tbl2 DEFAULT;
> > >
> > > INSERT INTO tbl2 DEFAULT VALUES;
> > > ERROR: no owned sequence found
> > >
> > > Though it works with tbl2(a int NOT NULL, b text)...
> > > Take a look at this too, please.
> > >
> >
> > Thanks Alexander for the report.
> >
> > PFA patch which fixes all the three problems.
> >
> > I had not fixed getIdentitySequence() to fetch identity sequence
> associated
> > with the partition because I thought it would be better to fail with an
> > error when it's not used correctly. But these bugs show 1. the error is
> > misleading and unwanted 2. there are more places where adding that logic
> > to getIdentitySequence() makes sense. Fixed the function in these
> patches.
> > Now callers like transformAlterTableStmt have to be careful not to call
> the
> > function on a partition.
> >
> > I have examined all the callers of getIdentitySequence() and they seem to
> > be fine. The code related to SetIdentity, DropIdentity is not called for
> > partitions, errors for which are thrown elsewhere earlier.
>
> Thanks for the fix.
>
> I had a quick look, it covers the issues mentioned above in the thread.
> Few nitpicks/questions:
>
> * I think it makes sense to verify if the ptup is valid. This approach
> would fail if the target column of the root partition is marked as
> attisdropped.
>
The column is searched by name which is derived from attno of child
partition. So it has to exist in the root partition. If it doesn't
something is seriously wrong. Do you have a reproducer? We may want to add
Assert(HeapTupleIsValid(ptup)) just in case. But it seems unnecessary to me.
>
> Oid
> -getIdentitySequence(Oid relid, AttrNumber attnum, bool missing_ok)
> +getIdentitySequence(Relation rel, AttrNumber attnum, bool missing_ok)
> {
>
> [...]
>
> + relid = llast_oid(ancestors);
> + ptup = SearchSysCacheAttName(relid, attname);
> + attnum = ((Form_pg_attribute) GETSTRUCT(ptup))->attnum;
>
> * getIdentitySequence is used in build_column_default, which in turn
> often appears in loops over table attributes. AFAICT it means that the
> same root partition search will be repeated multiple times in such
> situations if there is more than one identity. I assume the
> performance impact of this repetition is negligible?
>
I thought having multiple identity columns would be rare and hence avoided
making code complex. Otherwise we have to get root partition somewhere in
the caller hierarchy separately the logic much farther apart. Usually the
ancestor entries will be somewhere in the cache
>
> * Maybe a silly question, but since I'm not aware about all the details
> here, I'm curious -- the approach of mapping attributes of a partition
> to the root partition attributes, how robust is it? I guess there is
> no way that the root partition column will be not what is expected,
> e.g. due to some sort of concurrency?
>
Any such thing would require a lock on the partition relation in the
question which is locked before passing rel around? So it shouldn't happen.
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-05-06 14:01 Dmitry Dolgov <[email protected]>
parent: Ashutosh Bapat <[email protected]>
0 siblings, 0 replies; 31+ messages in thread
From: Dmitry Dolgov @ 2024-05-06 14:01 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Alexander Lakhin <[email protected]>; PostgreSQL Hackers <[email protected]>
> On Mon, May 06, 2024 at 06:52:41PM +0530, Ashutosh Bapat wrote:
> On Sun, May 5, 2024 at 1:43 AM Dmitry Dolgov <[email protected]> wrote:
> > I had a quick look, it covers the issues mentioned above in the thread.
> > Few nitpicks/questions:
> >
> > * I think it makes sense to verify if the ptup is valid. This approach
> > would fail if the target column of the root partition is marked as
> > attisdropped.
> >
>
> The column is searched by name which is derived from attno of child
> partition. So it has to exist in the root partition. If it doesn't
> something is seriously wrong. Do you have a reproducer? We may want to add
> Assert(HeapTupleIsValid(ptup)) just in case. But it seems unnecessary to me.
Sure, normally it should work. I don't have any particular situation in
mind, when attisdropped might be set on a root partition, but obviously
setting it manually crashes this path. Consider it mostly as suggestion
for a more defensive implementation "just in case".
> > Oid
> > -getIdentitySequence(Oid relid, AttrNumber attnum, bool missing_ok)
> > +getIdentitySequence(Relation rel, AttrNumber attnum, bool missing_ok)
> > {
> >
> > [...]
> >
> > + relid = llast_oid(ancestors);
> > + ptup = SearchSysCacheAttName(relid, attname);
> > + attnum = ((Form_pg_attribute) GETSTRUCT(ptup))->attnum;
> >
> > * getIdentitySequence is used in build_column_default, which in turn
> > often appears in loops over table attributes. AFAICT it means that the
> > same root partition search will be repeated multiple times in such
> > situations if there is more than one identity. I assume the
> > performance impact of this repetition is negligible?
> >
>
> I thought having multiple identity columns would be rare and hence avoided
> making code complex. Otherwise we have to get root partition somewhere in
> the caller hierarchy separately the logic much farther apart. Usually the
> ancestor entries will be somewhere in the cache
Yeah, agree, it's reasonable to expect that the case with multiple
identity columns will be rare.
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-05-07 21:04 Peter Eisentraut <[email protected]>
parent: Ashutosh Bapat <[email protected]>
2 siblings, 1 reply; 31+ messages in thread
From: Peter Eisentraut @ 2024-05-07 21:04 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; Alexander Lakhin <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On 30.04.24 12:59, Ashutosh Bapat wrote:
> PFA patch which fixes all the three problems.
I have committed this patch. Thanks all.
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: partitioning and identity column
@ 2024-05-09 20:32 Ashutosh Bapat <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 0 replies; 31+ messages in thread
From: Ashutosh Bapat @ 2024-05-09 20:32 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; PostgreSQL Hackers <[email protected]>
Thanks a lot Peter.
On Wed, May 8, 2024 at 2:34 AM Peter Eisentraut <[email protected]>
wrote:
> On 30.04.24 12:59, Ashutosh Bapat wrote:
> > PFA patch which fixes all the three problems.
>
> I have committed this patch. Thanks all.
>
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 31+ messages in thread
end of thread, other threads:[~2024-05-09 20:32 UTC | newest]
Thread overview: 31+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-01-11 06:00 [PATCH 4/4] Documentation for slot-limit feature Kyotaro Horiguchi <[email protected]>
2018-01-11 06:00 [PATCH 5/6] Documentation for slot-limit feature Kyotaro Horiguchi <[email protected]>
2018-01-11 06:00 [PATCH 4/4] Documentation for slot-limit feature Kyotaro Horiguchi <[email protected]>
2018-01-11 06:00 [PATCH 5/6] Documentation for slot-limit feature Kyotaro Horiguchi <[email protected]>
2018-01-11 06:00 [PATCH 4/4] Documentation for slot-limit feature Kyotaro Horiguchi <[email protected]>
2020-03-10 02:56 [PATCH v10 9/9] pg_ls_*dir to return all the metadata from pg_stat_file.. Justin Pryzby <[email protected]>
2024-01-16 18:59 Re: partitioning and identity column Peter Eisentraut <[email protected]>
2024-01-17 05:36 ` Re: partitioning and identity column Ashutosh Bapat <[email protected]>
2024-01-22 12:02 ` Re: partitioning and identity column Peter Eisentraut <[email protected]>
2024-01-22 12:23 ` Re: partitioning and identity column Ashutosh Bapat <[email protected]>
2024-01-22 18:59 ` Re: partitioning and identity column Peter Eisentraut <[email protected]>
2024-01-24 06:34 ` Re: partitioning and identity column Ashutosh Bapat <[email protected]>
2024-01-24 07:02 ` Re: partitioning and identity column Ashutosh Bapat <[email protected]>
2024-02-15 18:00 ` Re: partitioning and identity column Alexander Lakhin <[email protected]>
2024-02-19 12:17 ` Re: partitioning and identity column Ashutosh Bapat <[email protected]>
2024-02-19 15:00 ` Re: partitioning and identity column Alexander Lakhin <[email protected]>
2024-02-20 04:57 ` Re: partitioning and identity column Ashutosh Bapat <[email protected]>
2024-02-20 06:00 ` Re: partitioning and identity column Alexander Lakhin <[email protected]>
2024-02-21 00:51 ` Re: partitioning and identity column Alexander Korotkov <[email protected]>
2024-04-26 12:00 ` Re: partitioning and identity column Alexander Lakhin <[email protected]>
2024-04-26 12:57 ` Re: partitioning and identity column Ashutosh Bapat <[email protected]>
2024-04-26 18:00 ` Re: partitioning and identity column Alexander Lakhin <[email protected]>
2024-04-27 15:00 ` Re: partitioning and identity column Alexander Lakhin <[email protected]>
2024-04-28 06:59 ` Re: partitioning and identity column Alexander Lakhin <[email protected]>
2024-04-30 10:59 ` Re: partitioning and identity column Ashutosh Bapat <[email protected]>
2024-05-01 03:53 ` Re: partitioning and identity column Michael Paquier <[email protected]>
2024-05-04 20:13 ` Re: partitioning and identity column Dmitry Dolgov <[email protected]>
2024-05-06 13:22 ` Re: partitioning and identity column Ashutosh Bapat <[email protected]>
2024-05-06 14:01 ` Re: partitioning and identity column Dmitry Dolgov <[email protected]>
2024-05-07 21:04 ` Re: partitioning and identity column Peter Eisentraut <[email protected]>
2024-05-09 20:32 ` Re: partitioning and identity column Ashutosh Bapat <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox