Horton

Thread

Commits

  1. Revise MERGE documentation

  2. MERGE ... DO NOTHING: require SELECT privileges

  3. Rework query relation permission checking

  1. Running the fdw test from the terminal crashes into the core-dump

    Alena Rybakina <a.rybakina@postgrespro.ru> — 2024-02-18T18:33:07Z

    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

  2. Re: Running the fdw test from the terminal crashes into the core-dump

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-02-18T19:13:20Z

    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

  3. Re: Running the fdw test from the terminal crashes into the core-dump

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-02-18T19:44:02Z

    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)

  4. Re: Running the fdw test from the terminal crashes into the core-dump

    Amit Langote <amitlangote09@gmail.com> — 2024-02-19T00:03:58Z

    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

  5. Re: Running the fdw test from the terminal crashes into the core-dump

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-02-20T19:42:47Z

    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)

  6. Re: Running the fdw test from the terminal crashes into the core-dump

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-02-20T19:48:01Z

    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

  7. Re: Running the fdw test from the terminal crashes into the core-dump

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-02-21T16:53:07Z

    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)

  8. Re: Running the fdw test from the terminal crashes into the core-dump

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-02-22T23:48:20Z

    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

  9. Re: Running the fdw test from the terminal crashes into the core-dump

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-02-23T22:03:01Z

    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/

  10. Re: Running the fdw test from the terminal crashes into the core-dump

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-02-25T16:16:07Z

    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

  11. Re: Running the fdw test from the terminal crashes into the core-dump

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-02-26T15:13:11Z

    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)