Thread: 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)
Attachment
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)
Attachment
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
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 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. 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)