[PATCH] Fix memory corruption in pg_shdepend.c
Hi hackers,
One of our test runs under the memory sanitizer cathed [1]https://github.com/timescale/timescaledb/actions/runs/1343346998 the
following stacktrace:
```
heaptuple.c:1044:13: runtime error: load of value 111, which is not a
valid value for type '_Bool'
#0 0x55fbb5e0857b in heap_form_tuple
/home/runner/pgbuild/src/backend/access/common/heaptuple.c:1044
#1 0x55fbb679f62d in tts_heap_materialize
/home/runner/pgbuild/src/backend/executor/execTuples.c:381
#2 0x55fbb67addcf in ExecFetchSlotHeapTuple
/home/runner/pgbuild/src/backend/executor/execTuples.c:1654
#3 0x55fbb5f8127d in heap_multi_insert
/home/runner/pgbuild/src/backend/access/heap/heapam.c:2330
#4 0x55fbb6261b50 in CatalogTuplesMultiInsertWithInfo
/home/runner/pgbuild/src/backend/catalog/indexing.c:268
#5 0x55fbb62ce5aa in copyTemplateDependencies
/home/runner/pgbuild/src/backend/catalog/pg_shdepend.c:933
#6 0x55fbb650eb98 in createdb
/home/runner/pgbuild/src/backend/commands/dbcommands.c:590
#7 0x55fbb7062b30 in standard_ProcessUtility
/home/runner/pgbuild/src/backend/tcop/utility.c:773
#8 0x7fa942a63c13 in loader_process_utility_hook
/home/runner/work/timescaledb/timescaledb/src/loader/loader.c:522
#9 0x55fbb7063807 in ProcessUtility
/home/runner/pgbuild/src/backend/tcop/utility.c:523
#10 0x55fbb705bac3 in PortalRunUtility
/home/runner/pgbuild/src/backend/tcop/pquery.c:1147
#11 0x55fbb705c6fe in PortalRunMulti
/home/runner/pgbuild/src/backend/tcop/pquery.c:1304
#12 0x55fbb705d485 in PortalRun
/home/runner/pgbuild/src/backend/tcop/pquery.c:786
#13 0x55fbb704f613 in exec_simple_query
/home/runner/pgbuild/src/backend/tcop/postgres.c:1214
#14 0x55fbb7054b30 in PostgresMain
/home/runner/pgbuild/src/backend/tcop/postgres.c:4486
#15 0x55fbb6d78551 in BackendRun
/home/runner/pgbuild/src/backend/postmaster/postmaster.c:4506
#16 0x55fbb6d8334c in BackendStartup
/home/runner/pgbuild/src/backend/postmaster/postmaster.c:4228
#17 0x55fbb6d840cd in ServerLoop
/home/runner/pgbuild/src/backend/postmaster/postmaster.c:1745
#18 0x55fbb6d86611 in PostmasterMain
/home/runner/pgbuild/src/backend/postmaster/postmaster.c:1417
#19 0x55fbb6970b9b in main /home/runner/pgbuild/src/backend/main/main.c:209
```
It seems to be a bug in the PostgreSQL core. The memory corruption
happens @ pg_shdepend.c:914:
```
slot[slot_stored_count]->tts_values[Anum_pg_shdepend_refobjid
] = shdep->refobjid;
slot[slot_stored_count]->tts_values[Anum_pg_shdepend_deptype]
= shdep->deptype; <--- HERE
ExecStoreVirtualTuple(slot[slot_stored_count]);
```
The shdep->deptype value gets written to slot[0]->tts_isnull:
```
(lldb) p shdep->deptype
(char) $0 = 'o'
(lldb) p ((uint8_t*)slot[0]->tts_isnull)[0]
(uint8_t) $2 = 'o'
(lldb) p/d 'o'
(char) $4 = 111
```
I checked the rest of the PostgreSQL code and apparently, it should
have been tts_values[Anum_pg_shdepend_FOO - 1].
The patch is attached. The problem was first reported offlist by Sven
Klemm. Investigated and fixed by me.
[1]: https://github.com/timescale/timescaledb/actions/runs/1343346998
--
Best regards,
Aleksander Alekseev
Attachments:
v1-0001-memory-corruption-fix.patchapplication/octet-stream; name=v1-0001-memory-corruption-fix.patchDownload
diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c
index 4b676f5607..56a9a7662f 100644
--- a/src/backend/catalog/pg_shdepend.c
+++ b/src/backend/catalog/pg_shdepend.c
@@ -905,13 +905,13 @@ copyTemplateDependencies(Oid templateDbId, Oid newDbId)
shdep = (Form_pg_shdepend) GETSTRUCT(tup);
- slot[slot_stored_count]->tts_values[Anum_pg_shdepend_dbid] = ObjectIdGetDatum(newDbId);
- slot[slot_stored_count]->tts_values[Anum_pg_shdepend_classid] = shdep->classid;
- slot[slot_stored_count]->tts_values[Anum_pg_shdepend_objid] = shdep->objid;
- slot[slot_stored_count]->tts_values[Anum_pg_shdepend_objsubid] = shdep->objsubid;
- slot[slot_stored_count]->tts_values[Anum_pg_shdepend_refclassid] = shdep->refclassid;
- slot[slot_stored_count]->tts_values[Anum_pg_shdepend_refobjid] = shdep->refobjid;
- slot[slot_stored_count]->tts_values[Anum_pg_shdepend_deptype] = shdep->deptype;
+ slot[slot_stored_count]->tts_values[Anum_pg_shdepend_dbid - 1] = ObjectIdGetDatum(newDbId);
+ slot[slot_stored_count]->tts_values[Anum_pg_shdepend_classid - 1] = shdep->classid;
+ slot[slot_stored_count]->tts_values[Anum_pg_shdepend_objid - 1] = shdep->objid;
+ slot[slot_stored_count]->tts_values[Anum_pg_shdepend_objsubid - 1] = shdep->objsubid;
+ slot[slot_stored_count]->tts_values[Anum_pg_shdepend_refclassid - 1] = shdep->refclassid;
+ slot[slot_stored_count]->tts_values[Anum_pg_shdepend_refobjid - 1] = shdep->refobjid;
+ slot[slot_stored_count]->tts_values[Anum_pg_shdepend_deptype - 1] = shdep->deptype;
ExecStoreVirtualTuple(slot[slot_stored_count]);
slot_stored_count++;
On Wed, Oct 20, 2021 at 01:01:31PM +0300, Aleksander Alekseev wrote:
I checked the rest of the PostgreSQL code and apparently, it should
have been tts_values[Anum_pg_shdepend_FOO - 1].The patch is attached. The problem was first reported offlist by Sven
Klemm. Investigated and fixed by me.
Yes, that's indeed a one-off bug when copying shared dependencies of a
template database to the new one. This is new as of e3931d0, so I'll
take care of that and double-check the area while on.
--
Michael
On 20 Oct 2021, at 12:55, Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Oct 20, 2021 at 01:01:31PM +0300, Aleksander Alekseev wrote:
I checked the rest of the PostgreSQL code and apparently, it should
have been tts_values[Anum_pg_shdepend_FOO - 1].The patch is attached. The problem was first reported offlist by Sven
Klemm. Investigated and fixed by me.Yes, that's indeed a one-off bug when copying shared dependencies of a
template database to the new one. This is new as of e3931d0, so I'll
take care of that and double-check the area while on.
The attached patch looks correct to me. Skimming the referenced commit I see
nothing else sticking out.
--
Daniel Gustafsson https://vmware.com/
On 2021-Oct-20, Michael Paquier wrote:
On Wed, Oct 20, 2021 at 01:01:31PM +0300, Aleksander Alekseev wrote:
I checked the rest of the PostgreSQL code and apparently, it should
have been tts_values[Anum_pg_shdepend_FOO - 1].The patch is attached. The problem was first reported offlist by Sven
Klemm. Investigated and fixed by me.Yes, that's indeed a one-off bug when copying shared dependencies of a
template database to the new one. This is new as of e3931d0, so I'll
take care of that and double-check the area while on.
Ouch ... this means that pg_shdepends contents are broken for databases
created with 14.0? hmm ... yes.
alvherre=# create role rol;
CREATE ROLE
alvherre=# create table blarg() ;
CREATE TABLE
alvherre=# alter table blarg owner to rol;
ALTER TABLE
alvherre=# create database bar template alvherre;
CREATE DATABASE
alvherre=# \c bar
Ahora está conectado a la base de datos «bar» con el usuario «alvherre».
bar=# select * from pg_shdepend;
dbid | classid | objid | objsubid | refclassid | refobjid | deptype
-------+---------+-------+----------+------------+----------+---------
0 | 0 | 0 | 0 | 1260 | 10 | p
0 | 0 | 0 | 0 | 1260 | 6171 | p
0 | 0 | 0 | 0 | 1260 | 6181 | p
0 | 0 | 0 | 0 | 1260 | 6182 | p
0 | 0 | 0 | 0 | 1260 | 3373 | p
0 | 0 | 0 | 0 | 1260 | 3374 | p
0 | 0 | 0 | 0 | 1260 | 3375 | p
0 | 0 | 0 | 0 | 1260 | 3377 | p
0 | 0 | 0 | 0 | 1260 | 4569 | p
0 | 0 | 0 | 0 | 1260 | 4570 | p
0 | 0 | 0 | 0 | 1260 | 4571 | p
0 | 0 | 0 | 0 | 1260 | 4200 | p
12975 | 1259 | 37686 | 0 | 1260 | 37685 | o
| 37689 | 1259 | 37686 | 0 | 1260 | 5
(14 filas)
bar=# select 37689::regclass;
regclass
----------
37689
(1 fila)
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On Wed, Oct 20, 2021 at 09:19:51AM -0300, Alvaro Herrera wrote:
Ouch ... this means that pg_shdepends contents are broken for databases
created with 14.0? hmm ... yes.
Yes, it means so :(
I have fixed the issue for now, and monitored the rest of the tree.
Another issue is that we have zero coverage for this area of the code
when creating a database from a template and copying over shared
dependencies:
https://coverage.postgresql.org/src/backend/catalog/pg_shdepend.c.gcov.html
It is easy enough to get an error on the new database with
pg_describe_object(). Your part about adding a shared dependency with
a table on a given role is simple enough, as well. While looking for
a place where to put such a test, 020_createdb.pl felt like a natural
place and we don't have any coverage for the case of TEMPLATE with
createdb. So I would like to suggest something like the attached for
HEAD.
--
Michael
Attachments:
createdb-tap-deps.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/scripts/t/020_createdb.pl b/src/bin/scripts/t/020_createdb.pl
index 3db2f13ae2..5e03ac8937 100644
--- a/src/bin/scripts/t/020_createdb.pl
+++ b/src/bin/scripts/t/020_createdb.pl
@@ -6,7 +6,7 @@ use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 22;
+use Test::More tests => 25;
program_help_ok('createdb');
program_version_ok('createdb');
@@ -28,6 +28,28 @@ $node->issues_sql_like(
$node->command_fails([ 'createdb', 'foobar1' ],
'fails if database already exists');
+# Check use of templates with shared dependencies copied from the template.
+my ($ret, $stdout, $stderr) = $node->psql(
+ 'foobar2',
+ 'CREATE ROLE role_foobar;
+CREATE TABLE tab_foobar (id int);
+ALTER TABLE tab_foobar owner to role_foobar;');
+$node->issues_sql_like(
+ [ 'createdb', '-l', 'C', '-T', 'foobar2', 'foobar3' ],
+ qr/statement: CREATE DATABASE foobar3 TEMPLATE foobar2/,
+ 'create database with template');
+($ret, $stdout, $stderr) = $node->psql(
+ 'foobar3',
+ "SELECT pg_describe_object(classid, objid, objsubid) AS obj,
+ pg_describe_object(refclassid, refobjid, 0) AS refobj
+ FROM pg_shdepend s JOIN pg_database d ON (d.oid = s.dbid)
+ WHERE d.datname = 'foobar3';", on_error_die => 1);
+chomp($stdout);
+like(
+ $stdout,
+ qr/^tab_foobar|role role_foobar$/,
+ 'shared dependencies copied over to target database');
+
# Check quote handling with incorrect option values.
$node->command_checks_all(
[ 'createdb', '--encoding', "foo'; SELECT '1", 'foobar2' ],
On Wed, Oct 20, 2021 at 5:20 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
Ouch ... this means that pg_shdepends contents are broken for databases
created with 14.0? hmm ... yes.
I think that EDB's pg_catcheck tool can detect problems like this one.
Perhaps it can be converted into an amcheck/pg_amcheck patch, and
submitted. That would give us very broad coverage.
--
Peter Geoghegan
On Wed, Oct 20, 2021 at 07:59:50PM -0700, Peter Geoghegan wrote:
I think that EDB's pg_catcheck tool can detect problems like this one.
Yes, pg_catcheck is able to catch that.
Perhaps it can be converted into an amcheck/pg_amcheck patch, and
submitted. That would give us very broad coverage.
Perhaps. This means the creation of a new database with shared deps
in contrib/amcheck/t/. But is amcheck really a correct target here?
The fields involved here are an int, some OIDs and a char with a given
subset of values making them harder to check. pg_catcheck does checks
across catalogs, maintaining a mapping list as of its definitions.c.
--
Michael
On Wed, Oct 20, 2021 at 8:27 PM Michael Paquier <michael@paquier.xyz> wrote:
Perhaps. This means the creation of a new database with shared deps
in contrib/amcheck/t/. But is amcheck really a correct target here?
The fields involved here are an int, some OIDs and a char with a given
subset of values making them harder to check. pg_catcheck does checks
across catalogs, maintaining a mapping list as of its definitions.c.
Users should be able to use pg_amcheck as a high-level corruption
detection tool, which should include any new pg_catcheck style catalog
checking functionality. Whether or not we need to involve
contrib/amcheck itself doesn't seem important to me right now. Offhand
I think that we wouldn't, because as you point out pg_catcheck is a
frontend program that checks multiple databases.
--
Peter Geoghegan
On 2021-Oct-21, Michael Paquier wrote:
On Wed, Oct 20, 2021 at 09:19:51AM -0300, Alvaro Herrera wrote:
Ouch ... this means that pg_shdepends contents are broken for databases
created with 14.0? hmm ... yes.Yes, it means so :(
For the upcoming release notes in 14.1 I think we'd do well to document
how to find out if you're affected by this; and if you are, how to fix
it.
I suppose pg_describe_object can be used on the contents of pg_shdepend
to detect it. I'm less sure what to do to correct it -- delete the
bogus entries and regenerate them with some bulk query?
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Ninguna manada de bestias tiene una voz tan horrible como la humana" (Orual)
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
I suppose pg_describe_object can be used on the contents of pg_shdepend
to detect it. I'm less sure what to do to correct it -- delete the
bogus entries and regenerate them with some bulk query?
Seems that what copyTemplateDependencies wants to do can easily be
modeled by a SQL query, assuming you know which DB was cloned to
which other one, and that the source's shdeps didn't change since
then. However, I'm not sure how we can get rid of existing bogus
entries, especially if we'd like to preserve not-bogus ones
(which very likely have gotten added to the destination DB since
it was created).
On the whole I'm afraid that people messing with this manually are
likely to do more harm than good. pg_shdepend entries that don't
match any object probably won't cause a problem, and the lack of
protection against untimely dropping a role is unlikely to be much
of an issue for a role you're referencing in a template database.
So I suspect that practical issues will be rare. We're fortunate
that cloning a nonempty template database is rare already.
BTW, I think there is an additional bug in copyTemplateDependencies:
I do not see it initializing slot->tts_isnull[] anywhere. It
probably accidentally works (at least in devel builds) because we zero
that memory somewhere else, but surely this code shouldn't assume that?
regards, tom lane
On Thu, Oct 21, 2021 at 8:52 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
We're fortunate
that cloning a nonempty template database is rare already.
That, and a major use case for doing so is to quickly stage up testing data
in a new database (i.e., not a production use case). Though I could see
tenant-based products using this to bootstrap new clients I'd hope that is
a minority case.
David J.
Hi Tom,
BTW, I think there is an additional bug in copyTemplateDependencies:
I do not see it initializing slot->tts_isnull[] anywhere. It
probably accidentally works (at least in devel builds) because we zero
that memory somewhere else, but surely this code shouldn't assume that?
tts_isnull[] is zeroed in:
- copyTemplateDependencies
-- MakeSingleTupleTableSlot, which simply wraps:
--- MakeTupleTableSlot... where the slot is allocated with palloc0. The assumption that
MakeSingleTupleTableSlot() returns valid TupleTableSlot* with zeroed
tts_isnull[] seems reasonable, no?
What confuses me is the fact that we have two procedures that do the
same thing. Maybe one is redundant.
--
Best regards,
Aleksander Alekseev
On Fri, Oct 22, 2021 at 10:48:57AM +0300, Aleksander Alekseev wrote:
... where the slot is allocated with palloc0. The assumption that
MakeSingleTupleTableSlot() returns valid TupleTableSlot* with zeroed
tts_isnull[] seems reasonable, no?
Yes, I don't see any need to do something more here. The number of
arguments is fetched from the tuple descriptor itself, so the
allocation is sufficient.
What confuses me is the fact that we have two procedures that do the
same thing. Maybe one is redundant.
Do you have something in mind here?
Taking advantage of the catalog types and knowing that this is a
one-off, it is possible to recover dbid, classid, objid, objsubid and
refclassid. deptype can be mostly guessed from refclassid, but the
real problem is that refobjid is just lost because of the casting to a
char from and Oid.
[ ... Thinks more ... ]
Hmm. Wouldn't it be as simple as removing the entries in pg_shdepend
where dbid is NULL, and do an INSERT/SELECT with the existing entries
in pg_shdepend from the template database, updating dbid to the new
database? That would require users to know which template they used
as origin, as well as we could assume that no shared deps have changed
but that can be guessed by looking at all the misplaced fields. It is
true enough that users could make a lot of damage with chirurgy DMLs
on catalogs, though.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Fri, Oct 22, 2021 at 10:48:57AM +0300, Aleksander Alekseev wrote:
... where the slot is allocated with palloc0. The assumption that
MakeSingleTupleTableSlot() returns valid TupleTableSlot* with zeroed
tts_isnull[] seems reasonable, no?
Yes, I don't see any need to do something more here.
That assumption is exactly what I'm objecting to. I don't think
we make it in other places, and I don't like making it here.
(By "here" I mean all of e3931d0, because it made the same omission
in several places.)
The primary reason why I think it's a bad idea is that only one
path in MakeSingleTupleTableSlot provides a pre-zeroed tts_isnull
array --- if you don't supply a tuple descriptor at creation,
the assumption falls down. So even if this coding technique is
safe where it is, it is a hazard for anyone copying the code into
some other context.
I might be happier if we tried to guarantee that *every* way of
creating a slot will end with a pre-zeroed isnull array, and then
got rid of any thereby-duplicative memsets. But that would be
a lot more invasive than just making these places get in step.
regards, tom lane
Hi Michael,
Do you have something in mind here?
Yep. This is not a priority though, thus I created a separate CF entry:
https://commitfest.postgresql.org/35/3371/
--
Best regards,
Aleksander Alekseev
On 22 Oct 2021, at 15:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael@paquier.xyz> writes:
On Fri, Oct 22, 2021 at 10:48:57AM +0300, Aleksander Alekseev wrote:
... where the slot is allocated with palloc0. The assumption that
MakeSingleTupleTableSlot() returns valid TupleTableSlot* with zeroed
tts_isnull[] seems reasonable, no?Yes, I don't see any need to do something more here.
That assumption is exactly what I'm objecting to. I don't think
we make it in other places, and I don't like making it here.
(By "here" I mean all of e3931d0, because it made the same omission
in several places.)
The attached fixes the the two ones I spotted, are there any I missed?
Regardless of if we want to change the API (as discussed elsewhere here and in
a new thread), something like the attached should be done first and in 14 I
think.
--
Daniel Gustafsson https://vmware.com/
Attachments:
tts_isnull_zeroed.diffapplication/octet-stream; name=tts_isnull_zeroed.diff; x-unix-mode=0644Download
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 5898203972..81cc39fb70 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -764,6 +764,9 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
ExecClearTuple(slot[slotCount]);
+ memset(slot[slotCount]->tts_isnull, false,
+ slot[slotCount]->tts_tupleDescriptor->natts * sizeof(bool));
+
if (new_rel_oid != InvalidOid)
slot[slotCount]->tts_values[Anum_pg_attribute_attrelid - 1] = ObjectIdGetDatum(new_rel_oid);
else
diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c
index 56a9a7662f..8453d8fefd 100644
--- a/src/backend/catalog/pg_shdepend.c
+++ b/src/backend/catalog/pg_shdepend.c
@@ -903,6 +903,9 @@ copyTemplateDependencies(Oid templateDbId, Oid newDbId)
ExecClearTuple(slot[slot_stored_count]);
+ memset(slot[slot_stored_count]->tts_isnull, false,
+ slot[slot_stored_count]->tts_tupleDescriptor->natts * sizeof(bool));
+
shdep = (Form_pg_shdepend) GETSTRUCT(tup);
slot[slot_stored_count]->tts_values[Anum_pg_shdepend_dbid - 1] = ObjectIdGetDatum(newDbId);
Daniel Gustafsson <daniel@yesql.se> writes:
On 22 Oct 2021, at 15:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
(By "here" I mean all of e3931d0, because it made the same omission
in several places.)
The attached fixes the the two ones I spotted, are there any I missed?
Ah, you're right, InsertPgAttributeTuples is the only other spot in
that patch that's actually touching slots. I'd skimmed it a little
too quickly.
regards, tom lane
On 22 Oct 2021, at 20:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
On 22 Oct 2021, at 15:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
(By "here" I mean all of e3931d0, because it made the same omission
in several places.)The attached fixes the the two ones I spotted, are there any I missed?
Ah, you're right, InsertPgAttributeTuples is the only other spot in
that patch that's actually touching slots. I'd skimmed it a little
too quickly.
Thanks for confirming, unless there are objections I'll apply the fix to master
and backpatch to 14.
--
Daniel Gustafsson https://vmware.com/
On Fri, Oct 22, 2021 at 10:49:38PM +0200, Daniel Gustafsson wrote:
On 22 Oct 2021, at 20:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Ah, you're right, InsertPgAttributeTuples is the only other spot in
that patch that's actually touching slots. I'd skimmed it a little
too quickly.Thanks for confirming, unless there are objections I'll apply the fix to master
and backpatch to 14.
Fine by me. The patch looks OK.
--
Michael
On Thu, Oct 21, 2021 at 11:42:31AM +0900, Michael Paquier wrote:
It is easy enough to get an error on the new database with
pg_describe_object(). Your part about adding a shared dependency with
a table on a given role is simple enough, as well. While looking for
a place where to put such a test, 020_createdb.pl felt like a natural
place and we don't have any coverage for the case of TEMPLATE with
createdb. So I would like to suggest something like the attached for
HEAD.
I was thinking on this one over the last couple of days, and doing a
copy of shared deps from a template within createdb still feels
natural, as of this patch:
/messages/by-id/YXDTl+PfSnqmbbkE@paquier.xyz
Would somebody object to the addition of this test? Or perhaps
somebody has a better idea?
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
I was thinking on this one over the last couple of days, and doing a
copy of shared deps from a template within createdb still feels
natural, as of this patch:
/messages/by-id/YXDTl+PfSnqmbbkE@paquier.xyz
Would somebody object to the addition of this test? Or perhaps
somebody has a better idea?
I agree that we're not testing that area well enough. Proposed
patch seems basically OK, but I think the test needs to be stricter
about what the expected output looks like --- for instance, it
wouldn't complain if tab_foobar were described as something other
than a table.
regards, tom lane
On Mon, Oct 25, 2021 at 11:59:52AM -0400, Tom Lane wrote:
I agree that we're not testing that area well enough. Proposed
patch seems basically OK, but I think the test needs to be stricter
about what the expected output looks like --- for instance, it
wouldn't complain if tab_foobar were described as something other
than a table.
Indeed. There was also a problem in the regex itself, where '|' was
not escaped so the regex was not strict enough. While on it, I have
added a policy in the set copied to the new database. Testing the
case where the set of slots is full would require 2300~ entries, that
would take some time..
--
Michael
Attachments:
createdb-tap-deps-v2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/scripts/t/020_createdb.pl b/src/bin/scripts/t/020_createdb.pl
index cb8e26c773..6bcc59de08 100644
--- a/src/bin/scripts/t/020_createdb.pl
+++ b/src/bin/scripts/t/020_createdb.pl
@@ -6,7 +6,7 @@ use warnings;
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
-use Test::More tests => 22;
+use Test::More tests => 25;
program_help_ok('createdb');
program_version_ok('createdb');
@@ -28,6 +28,30 @@ $node->issues_sql_like(
$node->command_fails([ 'createdb', 'foobar1' ],
'fails if database already exists');
+# Check use of templates with shared dependencies copied from the template.
+my ($ret, $stdout, $stderr) = $node->psql(
+ 'foobar2',
+ 'CREATE ROLE role_foobar;
+CREATE TABLE tab_foobar (id int);
+ALTER TABLE tab_foobar owner to role_foobar;
+CREATE POLICY pol_foobar ON tab_foobar FOR ALL TO role_foobar;');
+$node->issues_sql_like(
+ [ 'createdb', '-l', 'C', '-T', 'foobar2', 'foobar3' ],
+ qr/statement: CREATE DATABASE foobar3 TEMPLATE foobar2/,
+ 'create database with template');
+($ret, $stdout, $stderr) = $node->psql(
+ 'foobar3',
+ "SELECT pg_describe_object(classid, objid, objsubid) AS obj,
+ pg_describe_object(refclassid, refobjid, 0) AS refobj
+ FROM pg_shdepend s JOIN pg_database d ON (d.oid = s.dbid)
+ WHERE d.datname = 'foobar3' ORDER BY obj;", on_error_die => 1);
+chomp($stdout);
+like(
+ $stdout,
+ qr/^policy pol_foobar on table tab_foobar\|role role_foobar
+table tab_foobar\|role role_foobar$/,
+ 'shared dependencies copied over to target database');
+
# Check quote handling with incorrect option values.
$node->command_checks_all(
[ 'createdb', '--encoding', "foo'; SELECT '1", 'foobar2' ],
On 23 Oct 2021, at 00:47, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Oct 22, 2021 at 10:49:38PM +0200, Daniel Gustafsson wrote:
On 22 Oct 2021, at 20:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Ah, you're right, InsertPgAttributeTuples is the only other spot in
that patch that's actually touching slots. I'd skimmed it a little
too quickly.Thanks for confirming, unless there are objections I'll apply the fix to master
and backpatch to 14.Fine by me. The patch looks OK.
Applied to master and 14, thanks.
--
Daniel Gustafsson https://vmware.com/
On Tue, Oct 26, 2021 at 02:43:26PM +0900, Michael Paquier wrote:
Indeed. There was also a problem in the regex itself, where '|' was
not escaped so the regex was not strict enough. While on it, I have
added a policy in the set copied to the new database. Testing the
case where the set of slots is full would require 2300~ entries, that
would take some time..
Applied this one as of 70bfc5a.
--
Michael