pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedBug when dumping "empty" operator classes
8+ messages / 5 participants
[nested] [flat]
* Bug when dumping "empty" operator classes
@ 2017-05-26 12:36 Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Gustafsson @ 2017-05-26 12:36 UTC (permalink / raw)
To: pgsql-hackers
While hacking on pg_upgrade in downstream Greenplum I ran into an error which
seems like an old, and obscure, bug in pg_dump (unrelated to pg_upgrade).
pg_dump generates incorrect SQL for an operator class which has no operators or
procedures, and which has the same column and storage types. While it’s
arguably a pretty uninteresting operator class, it is allowed since we don’t
validate that all required operators/procedures are present.
The alter_generic test suite contains examples of opclasses which trigger this
behavior. The below operator class:
CREATE OPERATOR CLASS alt_opc1 FOR TYPE uuid USING hash AS STORAGE uuid;
..is dumped like this (after an ALTER .. RENAME, thus the new name):
CREATE OPERATOR CLASS alt_opc3
FOR TYPE uuid USING hash FAMILY alt_opc1 AS
;
The reason why this hasn’t been caught by the PostgreSQL pg_upgrade test is
because the schema in which the operator classes are created is dropped at the
end of the suite, removing the DROP cause pg_upgrade to fail with:
pg_restore: [archiver (db)] could not execute query: ERROR: syntax error at or near ";"
LINE 3: ;
^
Command was: CREATE OPERATOR CLASS "alt_opc2"
FOR TYPE "macaddr" USING "hash" FAMILY "alt_opc2" AS
;
The attached patch adds a belts-and-suspenders check in dumpOpclass() which
appends the STORAGE clause in case nothing had been added. While adding
storage when it’s identical to the column type goes against the documentation,
it’s valid SQL and won’t break restore (and is handled by DefineOpClass()).
Validating the options fully would of course ensure that the dump always has
enough to render, but it also adds a lot of complexity (a quick look in the
archives doesn’t turn up many reports of it being a big problem). The DROP in
alter_generic is also removed to exercise the code path, being able to
pg_upgrade what is executed in regression seem like a good idea.
cheers ./daniel
Attachments:
opclass_pgdump.patch (application/octet-stream, 0B) download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Bug when dumping "empty" operator classes
@ 2017-05-26 15:08 Tom Lane <tgl@sss.pgh.pa.us>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 8+ messages in thread
From: Tom Lane @ 2017-05-26 15:08 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: pgsql-hackers
Daniel Gustafsson <daniel@yesql.se> writes:
> While hacking on pg_upgrade in downstream Greenplum I ran into an error which
> seems like an old, and obscure, bug in pg_dump (unrelated to pg_upgrade).
> pg_dump generates incorrect SQL for an operator class which has no operators or
> procedures, and which has the same column and storage types.
Good catch.
> The attached patch adds a belts-and-suspenders check in dumpOpclass() which
> appends the STORAGE clause in case nothing had been added.
Seems reasonable (the comment could use some wordsmithing maybe) ...
> ... The DROP in
> alter_generic is also removed to exercise the code path, being able to
> pg_upgrade what is executed in regression seem like a good idea.
... but that's a nonstarter. We can't have the regression tests leaving
global objects (users) lying around.
I'll commit and back-patch this without a test case. Possibly Frost will
be excited enough about it to add something to the pg_dump TAP tests,
but those tests are too opaque for me to want to do so.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Bug when dumping "empty" operator classes
@ 2017-05-26 15:14 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Gustafsson @ 2017-05-26 15:14 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
> On 26 May 2017, at 17:08, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> While hacking on pg_upgrade in downstream Greenplum I ran into an error which
>> seems like an old, and obscure, bug in pg_dump (unrelated to pg_upgrade).
>> pg_dump generates incorrect SQL for an operator class which has no operators or
>> procedures, and which has the same column and storage types.
>
> Good catch.
>
>> The attached patch adds a belts-and-suspenders check in dumpOpclass() which
>> appends the STORAGE clause in case nothing had been added.
>
> Seems reasonable (the comment could use some wordsmithing maybe) ...
>
>> ... The DROP in
>> alter_generic is also removed to exercise the code path, being able to
>> pg_upgrade what is executed in regression seem like a good idea.
>
> ... but that's a nonstarter. We can't have the regression tests leaving
> global objects (users) lying around.
Fair enough,
> I'll commit and back-patch this without a test case. Possibly Frost will
> be excited enough about it to add something to the pg_dump TAP tests,
> but those tests are too opaque for me to want to do so.
Thanks!
cheers ./daniel
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Bug when dumping "empty" operator classes
@ 2017-05-29 21:01 Michael Paquier <michael.paquier@gmail.com>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2017-05-29 21:01 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers; Stephen Frost <sfrost@snowman.net>; Daniel Gustafsson <daniel@yesql.se>
On Fri, May 26, 2017 at 8:14 AM, Daniel Gustafsson <daniel@yesql.se> wrote:
>> On 26 May 2017, at 17:08, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'll commit and back-patch this without a test case. Possibly Frost will
>> be excited enough about it to add something to the pg_dump TAP tests,
>> but those tests are too opaque for me to want to do so.
>
> Thanks!
For the TAP tests I think that you are looking for something like the
attached. Stephen, perhaps you could consider including this test in
the suite?
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments:
pgdump-tap-empty-opclass.patch (application/octet-stream, 1.8K) download
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index e70a421375..bf97342b87 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -3114,6 +3114,45 @@ qr/^\QCREATE DEFAULT CONVERSION test_conversion FOR 'LATIN1' TO 'UTF8' FROM iso8
section_data => 1,
section_post_data => 1, }, },
+ 'CREATE OPERATOR CLASS dump_test.op_class_empty' => {
+ all_runs => 1,
+ create_order => 74,
+ create_sql => 'CREATE OPERATOR CLASS dump_test.op_class_empty
+ FOR TYPE bigint USING btree FAMILY dump_test.op_family
+ AS STORAGE bigint;',
+ regexp => qr/^
+ \QCREATE OPERATOR CLASS op_class_empty\E\n\s+
+ \QFOR TYPE bigint USING btree FAMILY op_family AS\E\n\s+
+ \QSTORAGE bigint;\E
+ /xm,
+ like => {
+ binary_upgrade => 1,
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ exclude_test_table => 1,
+ exclude_test_table_data => 1,
+ no_blobs => 1,
+ no_privs => 1,
+ no_owner => 1,
+ only_dump_test_schema => 1,
+ pg_dumpall_dbprivs => 1,
+ schema_only => 1,
+ section_pre_data => 1,
+ test_schema_plus_blobs => 1,
+ with_oids => 1, },
+ unlike => {
+ column_inserts => 1,
+ data_only => 1,
+ exclude_dump_test_schema => 1,
+ only_dump_test_table => 1,
+ pg_dumpall_globals => 1,
+ pg_dumpall_globals_clean => 1,
+ role => 1,
+ section_data => 1,
+ section_post_data => 1, }, },
+
'CREATE EVENT TRIGGER test_event_trigger' => {
all_runs => 1,
create_order => 33,
* Re: [HACKERS] Bug when dumping "empty" operator classes
@ 2018-12-08 01:11 Stephen Frost <sfrost@snowman.net>
parent: Michael Paquier <michael.paquier@gmail.com>
0 siblings, 2 replies; 8+ messages in thread
From: Stephen Frost @ 2018-12-08 01:11 UTC (permalink / raw)
To: Michael Paquier <michael.paquier@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL mailing lists <pgsql-hackers@lists.postgresql.org>; Daniel Gustafsson <daniel@yesql.se>
Greetings,
* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Fri, May 26, 2017 at 8:14 AM, Daniel Gustafsson <daniel@yesql.se> wrote:
> >> On 26 May 2017, at 17:08, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I'll commit and back-patch this without a test case. Possibly Frost will
> >> be excited enough about it to add something to the pg_dump TAP tests,
> >> but those tests are too opaque for me to want to do so.
>
> For the TAP tests I think that you are looking for something like the
> attached. Stephen, perhaps you could consider including this test in
> the suite?
Another blast from the past. I've updated this to the new pg_dump
test-suite system but otherwise it's basically the same test.
Barring concerns, I'll plan to push this later this weekend.
Thanks!
Stephen
Attachments:
add-empty-op-class-pg_dump-test-v1_master.patch (text/x-diff, 1.0K) download
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
new file mode 100644
index 2afd950..c687f0f
*** a/src/bin/pg_dump/t/002_pg_dump.pl
--- b/src/bin/pg_dump/t/002_pg_dump.pl
*************** my %tests = (
*** 1521,1526 ****
--- 1521,1541 ----
unlike => { exclude_dump_test_schema => 1, },
},
+ 'CREATE OPERATOR CLASS dump_test.op_class_empty' => {
+ create_order => 89,
+ create_sql => 'CREATE OPERATOR CLASS dump_test.op_class_empty
+ FOR TYPE bigint USING btree FAMILY dump_test.op_family
+ AS STORAGE bigint;',
+ regexp => qr/^
+ \QCREATE OPERATOR CLASS dump_test.op_class_empty\E\n\s+
+ \QFOR TYPE bigint USING btree FAMILY dump_test.op_family AS\E\n\s+
+ \QSTORAGE bigint;\E
+ /xm,
+ like =>
+ { %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
+ unlike => { exclude_dump_test_schema => 1, },
+ },
+
'CREATE EVENT TRIGGER test_event_trigger' => {
create_order => 33,
create_sql => 'CREATE EVENT TRIGGER test_event_trigger
* Re: [HACKERS] Bug when dumping "empty" operator classes
@ 2018-12-08 03:54 Michael Paquier <michael@paquier.xyz>
parent: Stephen Frost <sfrost@snowman.net>
1 sibling, 0 replies; 8+ messages in thread
From: Michael Paquier @ 2018-12-08 03:54 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Michael Paquier <michael.paquier@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL mailing lists <pgsql-hackers@lists.postgresql.org>; Daniel Gustafsson <daniel@yesql.se>
On Fri, Dec 07, 2018 at 08:11:42PM -0500, Stephen Frost wrote:
> Another blast from the past. I've updated this to the new pg_dump
> test-suite system but otherwise it's basically the same test.
>
> Barring concerns, I'll plan to push this later this weekend.
Thanks Stephen for including this one. No objections from me, that
looks good.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [HACKERS] Bug when dumping "empty" operator classes
@ 2018-12-10 18:20 Stephen Frost <sfrost@snowman.net>
parent: Stephen Frost <sfrost@snowman.net>
1 sibling, 1 reply; 8+ messages in thread
From: Stephen Frost @ 2018-12-10 18:20 UTC (permalink / raw)
To: Michael Paquier <michael.paquier@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL mailing lists <pgsql-hackers@lists.postgresql.org>; Daniel Gustafsson <daniel@yesql.se>
Greetings,
* Stephen Frost (sfrost@snowman.net) wrote:
> * Michael Paquier (michael.paquier@gmail.com) wrote:
> > On Fri, May 26, 2017 at 8:14 AM, Daniel Gustafsson <daniel@yesql.se> wrote:
> > >> On 26 May 2017, at 17:08, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >> I'll commit and back-patch this without a test case. Possibly Frost will
> > >> be excited enough about it to add something to the pg_dump TAP tests,
> > >> but those tests are too opaque for me to want to do so.
> >
> > For the TAP tests I think that you are looking for something like the
> > attached. Stephen, perhaps you could consider including this test in
> > the suite?
>
> Another blast from the past. I've updated this to the new pg_dump
> test-suite system but otherwise it's basically the same test.
>
> Barring concerns, I'll plan to push this later this weekend.
This has now been pushed.
Thanks!
Stephen
Attachments:
signature.asc (application/pgp-signature, 818B) download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [HACKERS] Bug when dumping "empty" operator classes
@ 2018-12-11 05:28 Michael Paquier <michael@paquier.xyz>
parent: Stephen Frost <sfrost@snowman.net>
0 siblings, 0 replies; 8+ messages in thread
From: Michael Paquier @ 2018-12-11 05:28 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Michael Paquier <michael.paquier@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL mailing lists <pgsql-hackers@lists.postgresql.org>; Daniel Gustafsson <daniel@yesql.se>
On Mon, Dec 10, 2018 at 01:20:18PM -0500, Stephen Frost wrote:
> This has now been pushed.
Thanks for taking care of it, Stephen!
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2018-12-11 05:28 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2017-05-26 12:36 Bug when dumping "empty" operator classes Daniel Gustafsson <daniel@yesql.se>
2017-05-26 15:08 ` Tom Lane <tgl@sss.pgh.pa.us>
2017-05-26 15:14 ` Daniel Gustafsson <daniel@yesql.se>
2017-05-29 21:01 ` Michael Paquier <michael.paquier@gmail.com>
2018-12-08 01:11 ` Stephen Frost <sfrost@snowman.net>
2018-12-08 03:54 ` Michael Paquier <michael@paquier.xyz>
2018-12-10 18:20 ` Stephen Frost <sfrost@snowman.net>
2018-12-11 05:28 ` Michael Paquier <michael@paquier.xyz>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox