public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v16 09/10] pg_ls_*dir to return all the metadata from pg_stat_file..
3+ messages / 3 participants
[nested] [flat]
* [PATCH v16 09/10] 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; 3+ 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 | 65 +++++++++-----------
src/include/catalog/pg_proc.dat | 34 +++++-----
src/test/regress/expected/misc_functions.out | 16 ++---
src/test/regress/output/tablespace.source | 8 +--
5 files changed, 71 insertions(+), 82 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 3a2ad3eb50..1177799916 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25329,8 +25329,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>
@@ -25341,8 +25340,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>
@@ -25353,9 +25351,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>
@@ -25367,8 +25364,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.
@@ -25454,8 +25450,8 @@ LATERAL pg_ls_dir_recurse(dir) AS a;
</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.
@@ -25466,8 +25462,8 @@ LATERAL pg_ls_dir_recurse(dir) AS a;
</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>.
@@ -25479,8 +25475,8 @@ LATERAL pg_ls_dir_recurse(dir) AS a;
</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>.
@@ -25493,8 +25489,8 @@ LATERAL pg_ls_dir_recurse(dir) AS a;
<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 91bf8c69e9..10780d3fb1 100644
--- a/src/backend/utils/adt/genfile.c
+++ b/src/backend/utils/adt/genfile.c
@@ -36,6 +36,8 @@
#include "utils/syscache.h"
#include "utils/timestamp.h"
+static void tuple_from_stat(struct stat *fst, const char *path, Datum *values,
+ bool *isnull);
static Datum pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags);
#define LS_DIR_ISDIR (1<<0) /* Show column: isdir */
@@ -367,6 +369,28 @@ pg_read_binary_file_all(PG_FUNCTION_ARGS)
return pg_read_binary_file(fcinfo);
}
+/*
+ * Populate values and isnull from fst and path.
+ * Used for pg_stat_file() and pg_stat_dir_files()
+ * isnull is assumed to have been zeroed.
+ */
+static void
+tuple_from_stat(struct stat *fst, const char *path, Datum *values, bool *isnull)
+{
+ values[0] = Int64GetDatum((int64) fst->st_size);
+ values[1] = TimestampTzGetDatum(time_t_to_timestamptz(fst->st_atime));
+ values[2] = TimestampTzGetDatum(time_t_to_timestamptz(fst->st_mtime));
+ /* Unix has file status change time, while Win32 has creation time */
+#if !defined(WIN32) && !defined(__CYGWIN__)
+ values[3] = TimestampTzGetDatum(time_t_to_timestamptz(fst->st_ctime));
+ isnull[4] = true;
+#else
+ isnull[3] = true;
+ values[4] = TimestampTzGetDatum(time_t_to_timestamptz(fst->st_ctime));
+#endif
+ values[5] = BoolGetDatum(S_ISDIR(fst->st_mode));
+}
+
/*
* stat a file
*/
@@ -418,25 +442,7 @@ pg_stat_file(PG_FUNCTION_ARGS)
BlessTupleDesc(tupdesc);
memset(isnull, false, sizeof(isnull));
-
- values[0] = Int64GetDatum((int64) fst.st_size);
- values[1] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_atime));
- values[2] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_mtime));
- /* Unix has file status change time, while Win32 has creation time */
-#if !defined(WIN32) && !defined(__CYGWIN__)
- values[3] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_ctime));
- isnull[4] = true;
-#else
- isnull[3] = true;
- values[4] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_ctime));
-#endif
- values[5] = BoolGetDatum(S_ISDIR(fst.st_mode));
-#ifdef WIN32
- /* Links should have isdir=false */
- if (pgwin32_is_junction(filename))
- values[5] = BoolGetDatum(false);
-#endif
-
+ tuple_from_stat(&fst, filename, values, isnull);
tuple = heap_form_tuple(tupdesc, values, isnull);
pfree(filename);
@@ -578,8 +584,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;
@@ -618,23 +624,10 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags)
continue;
}
+ memset(nulls, 0, sizeof(nulls));
values[0] = CStringGetTextDatum(de->d_name);
if (flags & LS_DIR_METADATA)
- {
- values[1] = Int64GetDatum((int64) attrib.st_size);
- values[2] = TimestampTzGetDatum(time_t_to_timestamptz(attrib.st_mtime));
- if (flags & LS_DIR_ISDIR)
- {
- values[3] = BoolGetDatum(S_ISDIR(attrib.st_mode));
-#ifdef WIN32
- /* Links should have isdir=false */
- if (pgwin32_is_junction(path))
- values[3] = BoolGetDatum(false);
-#endif
- }
- }
-
- memset(nulls, 0, sizeof(nulls));
+ tuple_from_stat(&attrib, path, 1+values, 1+nulls);
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index f94f403475..34ee5a8f97 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10880,48 +10880,48 @@
{ 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' },
{ oid => '5033', descr => 'list directory with metadata',
proname => 'pg_ls_dir_metadata', procost => '10', prorows => '20', proretset => 't',
provolatile => 'v', prorettype => 'record', proargtypes => 'text',
- proallargtypes => '{text,text,int8,timestamptz,bool}', proargmodes => '{i,o,o,o,o}',
- proargnames => '{dirname,name,size,modification,isdir}',
+ proallargtypes => '{text,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{i,o,o,o,o,o,o,o}',
+ proargnames => '{dirname,name,size,access,modification,change,creation,isdir}',
prosrc => 'pg_ls_dir_metadata_1arg' },
{ oid => '5034', 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 ls as (select * from pg_ls_dir_metadata(dirname, true, false) union all select ls.name||'/'||a.name, a.size, a.modification, a.isdir from ls, lateral pg_ls_dir_metadata(dirname||'/'||ls.name, false, false)a where ls.isdir) select * from ls" },
+ 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 ls as (select * from pg_ls_dir_metadata(dirname, true, false) union all select ls.name||'/'||a.name, a.size, a.access, a.modification, a.change, a.creation, a.isdir from ls, lateral pg_ls_dir_metadata(dirname||'/'||ls.name, false, false)a where ls.isdir) select * from ls" },
# hash partitioning constraint function
{ oid => '5028', descr => 'hash partition CHECK constraint',
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index 4188d684f0..cce84a60a9 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;
@@ -222,8 +222,8 @@ ERROR: could not open directory "does not exist": No such file or directory
-- 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)
select name, isdir from pg_ls_dir_metadata('.') where name='.';
@@ -239,8 +239,8 @@ select name, isdir from pg_ls_dir_metadata('.', false, false) where name='.'; --
-- Check that expected columns are present
select * from pg_ls_dir_metadata('.') limit 0;
- name | size | modification | isdir
-------+------+--------------+-------
+ name | size | access | modification | change | creation | isdir
+------+------+--------+--------------+--------+----------+-------
(0 rows)
-- Check that we at least succeed in recursing once, and that we don't show the leading dir prefix
@@ -253,8 +253,8 @@ SELECT name, isdir FROM pg_ls_dir_recurse('.') WHERE isdir AND name~'^pg_wal';
-- Check that expected columns are present
SELECT * FROM pg_ls_dir_recurse('.') LIMIT 0;
- 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 ba9a3fe29a..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 | isdir
-------+------+--------------+-------
+ name | size | access | modification | change | creation | isdir
+------+------+--------+--------------+--------+----------+-------
(0 rows)
-- try setting and resetting some properties for the new tablespace
--
2.17.0
--2FkSFaIQeDFoAt0B
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v16-0010-pg_ls_-to-show-file-type-and-show-special-files.patch"
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Issues with ON CONFLICT UPDATE and REINDEX CONCURRENTLY
@ 2026-01-27 15:22 Álvaro Herrera <[email protected]>
2026-01-28 00:45 ` Re: Issues with ON CONFLICT UPDATE and REINDEX CONCURRENTLY Mihail Nikalayeu <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Álvaro Herrera @ 2026-01-27 15:22 UTC (permalink / raw)
To: Mihail Nikalayeu <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; Michael Paquier <[email protected]>; Noah Misch <[email protected]>; pgsql-hackers
On 2026-Jan-25, Mihail Nikalayeu wrote:
> Hello, Álvaro!
>
> Fixes are in attachment. I think the comment message and comments are good
> enough to explain the changes.
Actually, about this fragment ... if we track these ancestors for all
indexes, not just the ones that we consider as arbiters, don't we risk
doing something stupid when a completely unrelated index is being
reindexed? Namely, also consider that unrelated index as arbiter.
if (ancestors != NIL &&
!list_member_oid(ancestors_seen, linitial_oid(ancestors)))
{
foreach_oid(parent_idx, rootResultRelInfo->ri_onConflictArbiterIndexes)
{
if (list_member_oid(ancestors, parent_idx))
{
arbiterIndexes = lappend_oid(arbiterIndexes, indexoid);
arbiters_listidxs = lappend_int(arbiters_listidxs, listidx);
break;
}
}
/*
* Track which ancestor we saw, add other indexes that seem to have
* the same ancestor as "unparented".
*/
ancestors_seen = lappend_oid(ancestors_seen, linitial_oid(ancestors));
}
else
unparented_idxs = lappend_int(unparented_idxs, listidx);
I think the lappend_oid(ancestors_seen) should happen inside the
"if list_member_oid(ancestors)" block instead. I'm imagining a test
case like:
CREATE TABLE test.tblparted(i int primary key, updated_at timestamp) PARTITION BY RANGE (i);
CREATE TABLE test.tbl_partition PARTITION OF test.tblparted
FOR VALUES FROM (0) TO (10000)
WITH (parallel_workers = 0);
CREATE INDEX unrelated_index ON test.tblparted (updated_at);
where the reindex is
REINDEX INDEX CONCURRENTLY test.tbl_partition_updated_at_idx;
This doesn't fail, but I suspect it's just from me not setting up the
test case correctly.
I'm also missing a comment for why the use linitial_oid(ancestors) is
correct. At first I thought we should walk the entire ancestors list,
and do this dance if any OID there matches the ancestors_seen list. I
convinced myself that this isn't necessary because the scenario is a
single table being under REINDEX CONCURRENTLY, so the two indexes would
have the same root relation (top-most ancestor index). So comparing
linitial() is correct.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"La persona que no quería pecar / estaba obligada a sentarse
en duras y empinadas sillas / desprovistas, por cierto
de blandos atenuantes" (Patricio Vogel)
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Issues with ON CONFLICT UPDATE and REINDEX CONCURRENTLY
2026-01-27 15:22 Re: Issues with ON CONFLICT UPDATE and REINDEX CONCURRENTLY Álvaro Herrera <[email protected]>
@ 2026-01-28 00:45 ` Mihail Nikalayeu <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Mihail Nikalayeu @ 2026-01-28 00:45 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; Michael Paquier <[email protected]>; Noah Misch <[email protected]>; pgsql-hackers
Hello, Álvaro!
Thanks for your review!
> Actually, about this fragment ... if we track these ancestors for all
> indexes, not just the ones that we consider as arbiters, don't we risk
> doing something stupid when a completely unrelated index is being
> reindexed? Namely, also consider that unrelated index as arbiter.
Yes, such a thing may happen. It will not cause any error, because such an
index will be used as artiber only if another compatible index in that same
relation is present.
And additional_arbiters will be incremented.
> I think the lappend_oid(ancestors_seen) should happen inside the
> "if list_member_oid(ancestors)" block instead.
But yes, it is a more clear way. Added to v3.
> I'm also missing a comment for why the use linitial_oid(ancestors) is
> correct. At first I thought we should walk the entire ancestors list,
> and do this dance if any OID there matches the ancestors_seen list. I
> convinced myself that this isn't necessary because the scenario is a
> single table being under REINDEX CONCURRENTLY, so the two indexes would
> have the same root relation (top-most ancestor index). So comparing
> linitial() is correct.
Probably you think linitial() is "root", but it is an immediate parent.
I am not sure I understood you correctly, but I added a comment about it.
And about the opposite race + assert (not sure we need to keep it).
Best regards,
Mikhail.
Attachments:
[application/octet-stream] v3-0001-Fix-duplicate-arbiter-detection-during-REINDEX.patch (8.8K, ../../CADzfLwWLe9tYQZuK9CHTN+rHS4V64wjfhOuJnyXtPSVBhQUmtw@mail.gmail.com/3-v3-0001-Fix-duplicate-arbiter-detection-during-REINDEX.patch)
download | inline diff:
From 96a7cbf0edacfa527bf05eaef6f21aec537104dc Mon Sep 17 00:00:00 2001
From: Mikhail Nikalayeu <[email protected]>
Date: Wed, 28 Jan 2026 00:25:52 +0100
Subject: [PATCH v3] Fix duplicate arbiter detection during REINDEX
CONCURRENTLY on partitions MIME-Version: 1.0 Content-Type: text/plain;
charset=UTF-8 Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Commit 90eae926a fixed ON CONFLICT handling during REINDEX CONCURRENTLY
on partitioned tables by treating unparented indexes as potential
arbiters. However, there's a remaining race condition: when pg_inherits
records are swapped between consecutive calls to get_partition_ancestors(),
two different child indexes can appear to have the same parent, causing
duplicate entries in the arbiter list and triggering "invalid arbiter
index list" errors.
Note that this is not a new problem introduced by 90eae926a. The same
error could occur before that commit in a slightly different scenario:
an index is selected during planning, then index_concurrently_swap
commits, and a subsequent call to get_partition_ancestors() uses a new
catalog snapshot that sees zero ancestors for that index.
Fix by tracking which parent indexes have already been processed. If a
subsequent call to get_partition_ancestors() returns a parent we've
already seen, treat that index as unparented instead, allowing it to be
matched via IsIndexCompatibleAsArbiter() like other concurrent reindex
scenarios.
Author: Mihail Nikalayeu <[email protected]>
Reported-by: Alexander Lakhin <[email protected]>
Reviewed-by: Álvaro Herrera <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/executor/execPartition.c | 51 ++++++++++++++++---
.../t/010_index_concurrently_upsert.pl | 47 +++++++++++++++++
2 files changed, 90 insertions(+), 8 deletions(-)
diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index 55bdf5c4835..62dc5648d0f 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -28,6 +28,7 @@
#include "partitioning/partprune.h"
#include "rewrite/rewriteManip.h"
#include "utils/acl.h"
+#include "utils/injection_point.h"
#include "utils/lsyscache.h"
#include "utils/partcache.h"
#include "utils/rls.h"
@@ -761,12 +762,19 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
if (rootResultRelInfo->ri_onConflictArbiterIndexes != NIL)
{
List *unparented_idxs = NIL,
- *arbiters_listidxs = NIL;
+ *arbiters_listidxs = NIL,
+ *ancestors_seen = NIL;
for (int listidx = 0; listidx < leaf_part_rri->ri_NumIndices; listidx++)
{
Oid indexoid;
List *ancestors;
+ /* See below for handling the "same parent" case. */
+#ifdef USE_ASSERT_CHECKING
+ if (listidx > 0)
+ Assert(RelationGetRelid(leaf_part_rri->ri_IndexRelationDescs[listidx - 1]) <
+ RelationGetRelid(leaf_part_rri->ri_IndexRelationDescs[listidx]));
+#endif
/*
* If one of this index's ancestors is in the root's arbiter
@@ -775,17 +783,42 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
* in case REINDEX CONCURRENTLY is working on one of the
* arbiters.
*
- * XXX get_partition_ancestors is slow: it scans pg_inherits
- * each time. Consider a syscache or some other way to cache?
+ * However, if two indexes appear to have the same parent,
+ * treat the second of these as if it had no parent. This
+ * sounds counterintuitive, but it can happen if a transaction
+ * running REINDEX CONCURRENTLY commits right between those
+ * two indexes are checked by another process in this loop.
+ * This will have the effect of also treating that second
+ * index as arbiter.
+ *
+ * The opposite race (zero indexes with some parent) cannot
+ * happen: REINDEX CONCURRENTLY swaps only the pg_inherits
+ * link to the immediate parent (index_concurrently_swap).
+ * The new index always has a higher OID, so scanning in
+ * increasing OID order ensures we see at least one version
+ * with that parent. Because only that link is swapped, the
+ * immediate parent is the only ancestor that can differ
+ * between the two versions, so comparing linitial_oid is
+ * sufficient.
+ *
+ * XXX get_partition_ancestors scans pg_inherits, which is not
+ * only slow, but also means the catalog snapshot can get
+ * invalidated each time through the loop (cf.
+ * GetNonHistoricCatalogSnapshot). Consider a syscache or
+ * some other way to cache?
*/
indexoid = RelationGetRelid(leaf_part_rri->ri_IndexRelationDescs[listidx]);
ancestors = get_partition_ancestors(indexoid);
- if (ancestors != NIL)
+ INJECTION_POINT("exec-init-partition-after-get-partition-ancestors", NULL);
+
+ if (ancestors != NIL &&
+ !list_member_oid(ancestors_seen, linitial_oid(ancestors)))
{
foreach_oid(parent_idx, rootResultRelInfo->ri_onConflictArbiterIndexes)
{
if (list_member_oid(ancestors, parent_idx))
{
+ ancestors_seen = lappend_oid(ancestors_seen, linitial_oid(ancestors));
arbiterIndexes = lappend_oid(arbiterIndexes, indexoid);
arbiters_listidxs = lappend_int(arbiters_listidxs, listidx);
break;
@@ -794,6 +827,7 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
}
else
unparented_idxs = lappend_int(unparented_idxs, listidx);
+
list_free(ancestors);
}
@@ -812,16 +846,16 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
foreach_int(unparented_i, unparented_idxs)
{
Relation unparented_rel;
- IndexInfo *unparenred_ii;
+ IndexInfo *unparented_ii;
unparented_rel = leaf_part_rri->ri_IndexRelationDescs[unparented_i];
- unparenred_ii = leaf_part_rri->ri_IndexRelationInfo[unparented_i];
+ unparented_ii = leaf_part_rri->ri_IndexRelationInfo[unparented_i];
Assert(!list_member_oid(arbiterIndexes,
unparented_rel->rd_index->indexrelid));
/* Ignore indexes not ready */
- if (!unparenred_ii->ii_ReadyForInserts)
+ if (!unparented_ii->ii_ReadyForInserts)
continue;
foreach_int(arbiter_i, arbiters_listidxs)
@@ -839,7 +873,7 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
if (IsIndexCompatibleAsArbiter(arbiter_rel,
arbiter_ii,
unparented_rel,
- unparenred_ii))
+ unparented_ii))
{
arbiterIndexes = lappend_oid(arbiterIndexes,
unparented_rel->rd_index->indexrelid);
@@ -851,6 +885,7 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
}
list_free(unparented_idxs);
list_free(arbiters_listidxs);
+ list_free(ancestors_seen);
}
/*
diff --git a/src/test/modules/test_misc/t/010_index_concurrently_upsert.pl b/src/test/modules/test_misc/t/010_index_concurrently_upsert.pl
index 26682ebc55a..69178b129c7 100644
--- a/src/test/modules/test_misc/t/010_index_concurrently_upsert.pl
+++ b/src/test/modules/test_misc/t/010_index_concurrently_upsert.pl
@@ -785,6 +785,53 @@ clean_safe_quit_ok($s1, $s2, $s3);
$node->safe_psql('postgres', 'TRUNCATE TABLE test.tblexpr');
+############################################################################
+note('Test: REINDEX on partitioned table (invalid arbiter list)');
+
+# causes invalid ERROR: invalid arbiter index list
+
+$s1 = $node->background_psql('postgres', on_error_stop => 0);
+$s2 = $node->background_psql('postgres', on_error_stop => 0);
+$s3 = $node->background_psql('postgres', on_error_stop => 0);
+
+$s1->query_safe(
+ q[
+SELECT injection_points_set_local();
+SELECT injection_points_attach('exec-init-partition-after-get-partition-ancestors', 'wait');
+]);
+
+$s2->query_safe(
+ q[
+SELECT injection_points_set_local();
+SELECT injection_points_attach('reindex-relation-concurrently-before-swap', 'wait');
+]);
+
+$s2->query_until(
+ qr/starting_reindex/, q[
+\echo starting_reindex
+REINDEX INDEX CONCURRENTLY test.tbl_partition_pkey;
+]);
+
+ok_injection_point($node, 'reindex-relation-concurrently-before-swap');
+
+$s1->query_until(
+ qr/starting_upsert_s1/, q[
+\echo starting_upsert_s1
+INSERT INTO test.tblparted VALUES (13, now()) ON CONFLICT (i) DO UPDATE SET updated_at = now();
+]);
+
+ok_injection_point($node,
+ 'exec-init-partition-after-get-partition-ancestors');
+
+wakeup_injection_point($node, 'reindex-relation-concurrently-before-swap');
+
+wakeup_injection_point($node,
+ 'exec-init-partition-after-get-partition-ancestors');
+
+clean_safe_quit_ok($s1, $s2, $s3);
+
+$node->safe_psql('postgres', 'TRUNCATE TABLE test.tblparted');
+
done_testing();
############################################################################
--
2.43.0
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-01-28 00:45 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-10 02:56 [PATCH v16 09/10] pg_ls_*dir to return all the metadata from pg_stat_file.. Justin Pryzby <[email protected]>
2026-01-27 15:22 Re: Issues with ON CONFLICT UPDATE and REINDEX CONCURRENTLY Álvaro Herrera <[email protected]>
2026-01-28 00:45 ` Re: Issues with ON CONFLICT UPDATE and REINDEX CONCURRENTLY Mihail Nikalayeu <[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