Running the fdw test from the terminal crashes into the core-dump
Hi, hackers!
After starting the server (initdb + pg_ctl start) I ran a regress test
create_misc.sql ('\i src/test/regress/sql/create_misc.sql') and, after
that,
I ran the fdw test ('\i contrib/postgres_fdw/sql/postgres_fdw.sql') in
the psql, and it failed in the core-dump due to the worked assert.
To be honest, such a failure occurred only if the fdw extension was not
installed earlier.
script to reproduce the error:
./configure CFLAGS='-g -ggdb -O0' --enable-debug --enable-cassert
--prefix=`pwd`/tmp_install && make -j8 -s install
export CDIR=$(pwd)
export PGDATA=$CDIR/postgres_data
rm -r $PGDATA
mkdir $PGDATA
${CDIR}/tmp_install/bin/initdb -D $PGDATA >> log.txt
${CDIR}/tmp_install/bin/pg_ctl -D $PGDATA -l logfile start
${CDIR}/tmp_install/bin/psql -d postgres -f
src/test/regress/sql/create_misc.sql &&
${CDIR}/tmp_install/bin/psql -d postgres -f
contrib/postgres_fdw/sql/postgres_fdw.sql
The query, where the problem is:
-- MERGE ought to fail cleanly
merge into itrtest using (select 1, 'foo') as source on (true)
when matched then do nothing;
Coredump:
#5 0x0000555d1451f483 in ExceptionalCondition
(conditionName=0x555d146bba13 "requiredPerms != 0",
fileName=0x555d146bb7b0 "execMain.c",
lineNumber=654) at assert.c:66
#6 0x0000555d14064ebe in ExecCheckOneRelPerms (perminfo=0x555d1565ef90)
at execMain.c:654
#7 0x0000555d14064d94 in ExecCheckPermissions
(rangeTable=0x555d1565eef0, rteperminfos=0x555d1565efe0,
ereport_on_violation=true) at execMain.c:623
#8 0x0000555d140652ca in InitPlan (queryDesc=0x555d156cde50, eflags=0)
at execMain.c:850
#9 0x0000555d140644a8 in standard_ExecutorStart
(queryDesc=0x555d156cde50, eflags=0) at execMain.c:266
#10 0x0000555d140641ec in ExecutorStart (queryDesc=0x555d156cde50,
eflags=0) at execMain.c:145
#11 0x0000555d1432c025 in ProcessQuery (plan=0x555d1565f3e0,
sourceText=0x555d1551b020 "merge into itrtest using (select 1,
'foo') as source on (true)\n when matched then do nothing;", params=0x0,
queryEnv=0x0, dest=0x555d1565f540, qc=0x7fffc9454080) at pquery.c:155
#12 0x0000555d1432dbd8 in PortalRunMulti (portal=0x555d15597ef0,
isTopLevel=true, setHoldSnapshot=false, dest=0x555d1565f540,
altdest=0x555d1565f540,
qc=0x7fffc9454080) at pquery.c:1277
#13 0x0000555d1432d0cf in PortalRun (portal=0x555d15597ef0,
count=9223372036854775807, isTopLevel=true, run_once=true,
dest=0x555d1565f540,
altdest=0x555d1565f540, qc=0x7fffc9454080) at pquery.c:791
#14 0x0000555d14325ec8 in exec_simple_query (
--Type <RET> for more, q to quit, c to continue without paging--
query_string=0x555d1551b020 "merge into itrtest using (select 1,
'foo') as source on (true)\n when matched then do nothing;") at
postgres.c:1273
#15 0x0000555d1432ae4c in PostgresMain (dbname=0x555d15555ab0 "aaa",
username=0x555d15555a98 "alena") at postgres.c:4645
#16 0x0000555d14244a5d in BackendRun (port=0x555d1554b3b0) at
postmaster.c:4440
#17 0x0000555d14244072 in BackendStartup (port=0x555d1554b3b0) at
postmaster.c:4116
#18 0x0000555d142405c5 in ServerLoop () at postmaster.c:1768
#19 0x0000555d1423fec2 in PostmasterMain (argc=3, argv=0x555d1547fcf0)
at postmaster.c:1467
#20 0x0000555d140f3122 in main (argc=3, argv=0x555d1547fcf0) at main.c:198
This error is consistently reproduced.
To be honest, I wasn't able to fully figure out the reason for this, but
it seems that this operation on partitions should not be available at all?
alena@postgres=# SELECT relname, relkind FROM pg_class where
relname='itrtest';
relname | relkind
---------+---------
itrtest | p
(1 row)
--
Regards,
Alena Rybakina
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Alena Rybakina <a.rybakina@postgrespro.ru> writes:
After starting the server (initdb + pg_ctl start) I ran a regress test
create_misc.sql ('\i src/test/regress/sql/create_misc.sql') and, after
that,
I ran the fdw test ('\i contrib/postgres_fdw/sql/postgres_fdw.sql') in
the psql, and it failed in the core-dump due to the worked assert.
To be honest, such a failure occurred only if the fdw extension was not
installed earlier.
Thanks for the report! This can be reproduced more simply with
z=# create table test (a int, b text) partition by list(a);
CREATE TABLE
z=# merge into test using (select 1, 'foo') as source on (true) when matched then do nothing;
server closed the connection unexpectedly
The MERGE produces a query tree with
:rtable (
{RANGETBLENTRY
:alias <>
:eref
{ALIAS
:aliasname test
:colnames ("a" "b")
}
:rtekind 0
:relid 49152
:relkind p
:rellockmode 3
:tablesample <>
:perminfoindex 1
:lateral false
:inh true
:inFromCl false
:securityQuals <>
}
...
)
:rteperminfos (
{RTEPERMISSIONINFO
:relid 49152
:inh true
:requiredPerms 0
:checkAsUser 0
:selectedCols (b)
:insertedCols (b)
:updatedCols (b)
}
)
and that zero for requiredPerms is what leads to the assertion
failure later. So I'd blame this on faulty handling of the
zero-partitions case in the RTEPermissionInfo refactoring.
(I didn't bisect to prove that a61b1f748 is exactly where it
broke, but I do see that the query successfully does nothing
in v15.)
regards, tom lane
On 2024-Feb-18, Tom Lane wrote:
So I'd blame this on faulty handling of the zero-partitions case in
the RTEPermissionInfo refactoring. (I didn't bisect to prove that
a61b1f748 is exactly where it broke, but I do see that the query
successfully does nothing in v15.)
You're right, this is the commit that broke it. It's unclear to me if
Amit is available to look at it, so I'll give this a look tomorrow if he
isn't.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Porque francamente, si para saber manejarse a uno mismo hubiera que
rendir examen... ¿Quién es el machito que tendría carnet?" (Mafalda)
On Mon, Feb 19, 2024 at 4:44 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2024-Feb-18, Tom Lane wrote:
So I'd blame this on faulty handling of the zero-partitions case in
the RTEPermissionInfo refactoring. (I didn't bisect to prove that
a61b1f748 is exactly where it broke, but I do see that the query
successfully does nothing in v15.)You're right, this is the commit that broke it. It's unclear to me if
Amit is available to look at it, so I'll give this a look tomorrow if he
isn't.
I'll look at this today.
--
Thanks, Amit Langote
On 2024-Feb-18, Alvaro Herrera wrote:
On 2024-Feb-18, Tom Lane wrote:
So I'd blame this on faulty handling of the zero-partitions case in
the RTEPermissionInfo refactoring. (I didn't bisect to prove that
a61b1f748 is exactly where it broke, but I do see that the query
successfully does nothing in v15.)You're right, this is the commit that broke it. It's unclear to me if
Amit is available to look at it, so I'll give this a look tomorrow if he
isn't.
OK, so it turns out that the bug is older than that -- it was actually
introduced with MERGE itself (7103ebb7aae8) and has nothing to do with
partitioning or RTEPermissionInfo; commit a61b1f748 is only related
because it added the Assert() that barfs when there are no privileges to
check.
The real problem is that a MERGE ... DO NOTHING action reports that no
permissions need to be checked on the target relation, which is not a
problem when there are other actions in the MERGE command since they add
privs to check. When DO NOTHING is the only action, the added assert in
a61b1f748 is triggered.
So, this means we can fix this by simply requiring ACL_SELECT privileges
on a DO NOTHING action. We don't need to request specific privileges on
any particular column (perminfo->selectedCols continues to be the empty
set) -- which means that any role that has privileges on *any* column
would get a pass. If you're doing MERGE with any other action besides
DO NOTHING, you already have privileges on at least some column, so
adding ACL_SELECT breaks nothing.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Pido que me den el Nobel por razones humanitarias" (Nicanor Parra)
Attachments:
0001-MERGE-.-DO-NOTHING-require-SELECT-privileges.patchtext/x-diff; charset=utf-8Download
From 5f2eb9992f40933aa58f8bb0e0bed7ebd99f2952 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Tue, 20 Feb 2024 18:30:37 +0100
Subject: [PATCH] MERGE ... DO NOTHING: require SELECT privileges
Verify that a user running MERGE with a DO NOTHING clause has
privileges to read the table, even if no columns are referenced. Such
privileges were already required if the ON clause or any of the WHEN
conditions referenced any column at all, so there's no functional change
in practice.
This change fixes an assertion failure in the case where no column is
referenced by the command and the WHEN clauses are all DO NOTHING.
Backpatch to 15, where MERGE was introduced.
Reported-by: Alena Rybakina <a.rybakina@postgrespro.ru>
Discussion: https://postgr.es/m/4d65a385-7efa-4436-a825-0869f89d9d92@postgrespro.ru
---
src/backend/parser/parse_merge.c | 7 ++++++-
src/test/regress/expected/merge.out | 11 ++++++++++-
src/test/regress/sql/merge.sql | 11 +++++++++++
3 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/src/backend/parser/parse_merge.c b/src/backend/parser/parse_merge.c
index 5f6a683ab9..73f7a48b3c 100644
--- a/src/backend/parser/parse_merge.c
+++ b/src/backend/parser/parse_merge.c
@@ -133,7 +133,11 @@ transformMergeStmt(ParseState *pstate, MergeStmt *stmt)
int when_type = (mergeWhenClause->matched ? 0 : 1);
/*
- * Collect action types so we can check target permissions
+ * Collect permissions to check, according to action types. We require
+ * SELECT privileges for DO NOTHING because it'd be irregular to have
+ * a target relation with zero privileges checked, in case DO NOTHING
+ * is the only action. There's no damage from that: any meaningful
+ * MERGE command requires at least some access to the table anyway.
*/
switch (mergeWhenClause->commandType)
{
@@ -147,6 +151,7 @@ transformMergeStmt(ParseState *pstate, MergeStmt *stmt)
targetPerms |= ACL_DELETE;
break;
case CMD_NOTHING:
+ targetPerms |= ACL_SELECT;
break;
default:
elog(ERROR, "unknown action in MERGE WHEN clause");
diff --git a/src/test/regress/expected/merge.out b/src/test/regress/expected/merge.out
index f87905fabd..125d7b6bca 100644
--- a/src/test/regress/expected/merge.out
+++ b/src/test/regress/expected/merge.out
@@ -3,6 +3,7 @@
--
CREATE USER regress_merge_privs;
CREATE USER regress_merge_no_privs;
+CREATE USER regress_merge_none;
DROP TABLE IF EXISTS target;
NOTICE: table "target" does not exist, skipping
DROP TABLE IF EXISTS source;
@@ -159,12 +160,19 @@ ERROR: cannot execute MERGE on relation "mv"
DETAIL: This operation is not supported for materialized views.
DROP MATERIALIZED VIEW mv;
-- permissions
+SET SESSION AUTHORIZATION regress_merge_none;
+MERGE INTO target
+USING (SELECT 1)
+ON true
+WHEN MATCHED THEN
+ DO NOTHING;
+ERROR: permission denied for table target
+RESET SESSION AUTHORIZATION;
MERGE INTO target
USING source2
ON target.tid = source2.sid
WHEN MATCHED THEN
UPDATE SET balance = 0;
-ERROR: permission denied for table source2
GRANT INSERT ON target TO regress_merge_no_privs;
SET SESSION AUTHORIZATION regress_merge_no_privs;
MERGE INTO target
@@ -2248,3 +2256,4 @@ DROP TABLE source, source2;
DROP FUNCTION merge_trigfunc();
DROP USER regress_merge_privs;
DROP USER regress_merge_no_privs;
+DROP USER regress_merge_none;
diff --git a/src/test/regress/sql/merge.sql b/src/test/regress/sql/merge.sql
index 66cb75a756..b5bddfcf8d 100644
--- a/src/test/regress/sql/merge.sql
+++ b/src/test/regress/sql/merge.sql
@@ -4,6 +4,8 @@
CREATE USER regress_merge_privs;
CREATE USER regress_merge_no_privs;
+CREATE USER regress_merge_none;
+
DROP TABLE IF EXISTS target;
DROP TABLE IF EXISTS source;
CREATE TABLE target (tid integer, balance integer)
@@ -118,6 +120,14 @@ DROP MATERIALIZED VIEW mv;
-- permissions
+SET SESSION AUTHORIZATION regress_merge_none;
+MERGE INTO target
+USING (SELECT 1)
+ON true
+WHEN MATCHED THEN
+ DO NOTHING;
+RESET SESSION AUTHORIZATION;
+
MERGE INTO target
USING source2
ON target.tid = source2.sid
@@ -1471,3 +1481,4 @@ DROP TABLE source, source2;
DROP FUNCTION merge_trigfunc();
DROP USER regress_merge_privs;
DROP USER regress_merge_no_privs;
+DROP USER regress_merge_none;
--
2.39.2
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
The real problem is that a MERGE ... DO NOTHING action reports that no
permissions need to be checked on the target relation, which is not a
problem when there are other actions in the MERGE command since they add
privs to check. When DO NOTHING is the only action, the added assert in
a61b1f748 is triggered.
So, this means we can fix this by simply requiring ACL_SELECT privileges
on a DO NOTHING action. We don't need to request specific privileges on
any particular column (perminfo->selectedCols continues to be the empty
set) -- which means that any role that has privileges on *any* column
would get a pass. If you're doing MERGE with any other action besides
DO NOTHING, you already have privileges on at least some column, so
adding ACL_SELECT breaks nothing.
LGTM.
regards, tom lane
On 2024-Feb-20, Tom Lane wrote:
So, this means we can fix this by simply requiring ACL_SELECT privileges
on a DO NOTHING action. We don't need to request specific privileges on
any particular column (perminfo->selectedCols continues to be the empty
set) -- which means that any role that has privileges on *any* column
would get a pass.LGTM.
Thanks for looking!
After having pushed that, I wonder if we should document this. It seems
quite the minor thing, but I'm sure somebody will complain if we don't.
I propose the attached. (Extra context so that the full paragraph can
be read from the comfort of your email program.)
(While at it, I found the placement of the previous-to-last sentence in
that paragraph rather strange, so I moved it to the end.)
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Sallah, I said NO camels! That's FIVE camels; can't you count?"
(Indiana Jones)
Attachments:
mergepriv-doc.patchtext/x-diff; charset=utf-8Download
diff --git a/doc/src/sgml/ref/merge.sgml b/doc/src/sgml/ref/merge.sgml
index 655f7dcc05..85938eda07 100644
--- a/doc/src/sgml/ref/merge.sgml
+++ b/doc/src/sgml/ref/merge.sgml
@@ -97,26 +97,29 @@ DELETE
<para>
There is no separate <literal>MERGE</literal> privilege.
If you specify an update action, you must have the
<literal>UPDATE</literal> privilege on the column(s)
of the <replaceable class="parameter">target_table_name</replaceable>
that are referred to in the <literal>SET</literal> clause.
If you specify an insert action, you must have the <literal>INSERT</literal>
privilege on the <replaceable class="parameter">target_table_name</replaceable>.
If you specify a delete action, you must have the <literal>DELETE</literal>
privilege on the <replaceable class="parameter">target_table_name</replaceable>.
- Privileges are tested once at statement start and are checked
- whether or not particular <literal>WHEN</literal> clauses are executed.
You will require the <literal>SELECT</literal> privilege on any column(s)
of the <replaceable class="parameter">data_source</replaceable> and
<replaceable class="parameter">target_table_name</replaceable> referred to
- in any <literal>condition</literal> or <literal>expression</literal>.
+ in any <literal>condition</literal> or <literal>expression</literal>;
+ in addition, if a <literal>DO NOTHING</literal> action is specified, you
+ will require the <literal>SELECT</literal> privilege on at least one column
+ of <replaceable class="parameter">target_table_name</replaceable>.
+ Privileges are tested once at statement start and are checked
+ whether or not particular <literal>WHEN</literal> clauses are executed.
</para>
<para>
<command>MERGE</command> is not supported if the
<replaceable class="parameter">target_table_name</replaceable> is a
materialized view, foreign table, or if it has any
rules defined on it.
</para>
</refsect1>
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
After having pushed that, I wonder if we should document this. It seems
quite the minor thing, but I'm sure somebody will complain if we don't.
Yup, no doubt.
I propose the attached. (Extra context so that the full paragraph can
be read from the comfort of your email program.)
This reads awkwardly to me. I think it'd be better to construct it
so that DO NOTHING's requirement is stated exactly parallel to the other
three clause types, more or less as attached.
(While at it, I found the placement of the previous-to-last sentence in
that paragraph rather strange, so I moved it to the end.)
Agreed, and done in my version too.
BTW, if you read it without paying attention to markup, you'll notice
that we are saying things like
If you specify an insert action, you must have the INSERT
privilege on the target_table_name.
which is fairly nonsensical: we don't define privileges on the name
of something. While I've not done anything about that here,
I wonder if we shouldn't just write "privilege on the target table"
without any special markup.
regards, tom lane
Attachments:
merge-privilege-doc.patchtext/x-diff; charset=us-ascii; name=merge-privilege-doc.patchDownload
diff --git a/doc/src/sgml/ref/merge.sgml b/doc/src/sgml/ref/merge.sgml
index 655f7dcc05..79ebff1033 100644
--- a/doc/src/sgml/ref/merge.sgml
+++ b/doc/src/sgml/ref/merge.sgml
@@ -104,12 +104,15 @@ DELETE
privilege on the <replaceable class="parameter">target_table_name</replaceable>.
If you specify a delete action, you must have the <literal>DELETE</literal>
privilege on the <replaceable class="parameter">target_table_name</replaceable>.
- Privileges are tested once at statement start and are checked
- whether or not particular <literal>WHEN</literal> clauses are executed.
- You will require the <literal>SELECT</literal> privilege on any column(s)
+ If you specify a <literal>DO NOTHING</literal> action, you must have
+ the <literal>SELECT</literal> privilege on at least one column
+ of <replaceable class="parameter">target_table_name</replaceable>.
+ You will also need <literal>SELECT</literal> privilege on any column(s)
of the <replaceable class="parameter">data_source</replaceable> and
<replaceable class="parameter">target_table_name</replaceable> referred to
in any <literal>condition</literal> or <literal>expression</literal>.
+ Privileges are tested once at statement start and are checked
+ whether or not particular <literal>WHEN</literal> clauses are executed.
</para>
<para>
On 2024-Feb-22, Tom Lane wrote:
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
I propose the attached. (Extra context so that the full paragraph can
be read from the comfort of your email program.)This reads awkwardly to me. I think it'd be better to construct it
so that DO NOTHING's requirement is stated exactly parallel to the other
three clause types, more or less as attached.
Sure, that works.
BTW, if you read it without paying attention to markup, you'll notice
that we are saying things likeIf you specify an insert action, you must have the INSERT
privilege on the target_table_name.which is fairly nonsensical: we don't define privileges on the name
of something.
Hmm, you're right, this is not strictly correct.
While I've not done anything about that here, I wonder if we shouldn't
just write "privilege on the target table" without any special markup.
That would work for me.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
On 2024-Feb-22, Tom Lane wrote:
While I've not done anything about that here, I wonder if we shouldn't
just write "privilege on the target table" without any special markup.
That would work for me.
OK. Will you do the honors, or shall I?
regards, tom lane
On 2024-Feb-25, Tom Lane wrote:
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
On 2024-Feb-22, Tom Lane wrote:
While I've not done anything about that here, I wonder if we shouldn't
just write "privilege on the target table" without any special markup.That would work for me.
OK. Will you do the honors, or shall I?
I can push the whole later today.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Linux transformó mi computadora, de una `máquina para hacer cosas',
en un aparato realmente entretenido, sobre el cual cada día aprendo
algo nuevo" (Jaime Salinas)