pg_get_constraintdef: Schema qualify foreign tables unless pretty printing is enabled
Hi hackers,
Whilst debugging an issue with the output of pg_get_constraintdef, we've
discovered that pg_get_constraintdef doesn't schema qualify foreign tables
mentioned in the REFERENCES clause, even if pretty printing
(PRETTYFLAG_SCHEMA) is turned off.
This is a problem because it means there is no way to get a constraint
definition that can be recreated on another system when multiple schemas
are in use, but a different search_path is set. It's also different from
pg_get_indexdef, where this flag is correctly respected.
I assume this is an oversight, since the fix is pretty straightforward, see
attached patch. I'll register the patch for the next commitfest.
Here is a test case from my colleague Maciek showing this difference:
create schema s;
create table s.foo(a int primary key);
create table s.bar(a int primary key, b int references s.foo(a));
select pg_get_indexdef(indexrelid, 0, false) from pg_index order by
indexrelid desc limit 3;
pg_get_indexdef
-------------------------------------------------------------------------------------------------------
CREATE UNIQUE INDEX bar_pkey ON s.bar USING btree (a)
CREATE UNIQUE INDEX foo_pkey ON s.foo USING btree (a)
CREATE UNIQUE INDEX pg_toast_13593_index ON pg_toast.pg_toast_13593 USING
btree (chunk_id, chunk_seq)
(3 rows)
select pg_get_constraintdef(oid, false) from pg_constraint order by oid
desc limit 3;
pg_get_constraintdef
-----------------------------------
FOREIGN KEY (b) REFERENCES foo(a)
PRIMARY KEY (a)
PRIMARY KEY (a)
(3 rows)
Thanks,
Lukas
--
Lukas Fittl
Attachments:
v1-0001-pg_get_constraintdef-Schema-qualify-foreign-table.patchapplication/x-patch; name=v1-0001-pg_get_constraintdef-Schema-qualify-foreign-table.patchDownload
From 83a1ab6081f70d0eed820b2b13bc3ab6e93af8ec Mon Sep 17 00:00:00 2001
From: Lukas Fittl <lukas@fittl.com>
Date: Tue, 9 Aug 2022 16:55:35 -0700
Subject: [PATCH v1] pg_get_constraintdef: Schema qualify foreign tables by
default
This matches pg_get_constraintdef to behave the same way as
pg_get_indexdef, which is to schema qualify referenced objects in the
definition, unless pretty printing is explicitly requested.
For pretty printing the previous behaviour is retained, which is to only
include the schema information if the referenced object is not in the
current search path.
---
src/backend/utils/adt/ruleutils.c | 5 +++--
src/test/regress/expected/foreign_key.out | 8 ++++----
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index d575aa0066..b7a2a356b4 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -2249,8 +2249,9 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
/* add foreign relation name */
appendStringInfo(&buf, ") REFERENCES %s(",
- generate_relation_name(conForm->confrelid,
- NIL));
+ (prettyFlags & PRETTYFLAG_SCHEMA) ?
+ generate_relation_name(conForm->confrelid, NIL) :
+ generate_qualified_relation_name(conForm->confrelid));
/* Fetch and build referenced-column list */
val = SysCacheGetAttr(CONSTROID, tup,
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index da26f083bc..a70f7c2491 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -775,10 +775,10 @@ CREATE TABLE FKTABLE (
FOREIGN KEY (tid, fk_id_del_set_default) REFERENCES PKTABLE ON DELETE SET DEFAULT (fk_id_del_set_default)
);
SELECT pg_get_constraintdef(oid) FROM pg_constraint WHERE conrelid = 'fktable'::regclass::oid ORDER BY oid;
- pg_get_constraintdef
---------------------------------------------------------------------------------------------------------------------
- FOREIGN KEY (tid, fk_id_del_set_null) REFERENCES pktable(tid, id) ON DELETE SET NULL (fk_id_del_set_null)
- FOREIGN KEY (tid, fk_id_del_set_default) REFERENCES pktable(tid, id) ON DELETE SET DEFAULT (fk_id_del_set_default)
+ pg_get_constraintdef
+---------------------------------------------------------------------------------------------------------------------------
+ FOREIGN KEY (tid, fk_id_del_set_null) REFERENCES public.pktable(tid, id) ON DELETE SET NULL (fk_id_del_set_null)
+ FOREIGN KEY (tid, fk_id_del_set_default) REFERENCES public.pktable(tid, id) ON DELETE SET DEFAULT (fk_id_del_set_default)
(2 rows)
INSERT INTO PKTABLE VALUES (1, 0), (1, 1), (1, 2);
--
2.34.0
Lukas Fittl <lukas@fittl.com> writes:
Whilst debugging an issue with the output of pg_get_constraintdef, we've
discovered that pg_get_constraintdef doesn't schema qualify foreign tables
mentioned in the REFERENCES clause, even if pretty printing
(PRETTYFLAG_SCHEMA) is turned off.
This is a problem because it means there is no way to get a constraint
definition that can be recreated on another system when multiple schemas
are in use, but a different search_path is set. It's also different from
pg_get_indexdef, where this flag is correctly respected.
I would say that pg_get_indexdef is the one that's out of step.
I count 11 calls of generate_relation_name in ruleutils.c,
of which only three have this business of being overridden
when not-pretty. What is the rationale for that, and why
would we move pg_get_constraintdef from one category to the
other?
regards, tom lane
On Tue, Aug 9, 2022 at 5:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I would say that pg_get_indexdef is the one that's out of step.
I count 11 calls of generate_relation_name in ruleutils.c,
of which only three have this business of being overridden
when not-pretty. What is the rationale for that, and why
would we move pg_get_constraintdef from one category to the
other?
The overall motivation here is to make it easy to recreate the schema
without having to match the search_path on the importing side to be
identical to the exporting side. There is a workaround, which is to do a
SET search_path before calling these functions that excludes the referenced
schemas (which I guess is what pg_dump does?).
But I wonder, why do we have an explicit pretty printing flag on these
functions, and PRETTYFLAG_SCHEMA in the code to represent this behavior. If
we don't want pretty printing to affect schema qualification, why does that
flag exist?
Of the other call sites, in terms of using generate_relation_name vs
generate_qualified_relation_name:
* pg_get_triggerdef_worker makes it conditional on pretty=true, but only
for ON, not the FROM (not clear why that difference exists?)
* pg_get_indexdef_worker makes it conditional on prettyFlags &
PRETTYFLAG_SCHEMA for the ON
* pg_get_statisticsobj_worker does not handle pretty printing (always uses
generate_relation_name)
* make_ruledef makes it conditional on prettyFlags & PRETTYFLAG_SCHEMA for
the TO
* get_insert_query_def does not handle pretty printing (always uses
generate_relation_name)
* get_update_query_def does not handle pretty printing (always uses
generate_relation_name)
* get_delete_query_def does not handle pretty printing (always uses
generate_relation_name)
* get_rule_expr does not handle pretty printing (always uses
generate_relation_name)
* get_from_clause_item does not handle pretty printing (always uses
generate_relation_name)
Looking at that, it seems we didn't make the effort for the view related
code with all its complexity, and didn't do it for pg_get_statisticsobjdef
since it doesn't have a pretty flag. Why we didn't do it in
pg_get_triggerdef_worker for FROM isn't clear to me.
If we want to be entirely consistent (and keep supporting
PRETTYFLAG_SCHEMA), that probably means:
* Adding a pretty flag to pg_get_statisticsobjdef
* Teaching get_query_def to pass down prettyFlags to get_*_query_def
functions
* Update pg_get_triggerdef_worker to handle pretty for FROM as well
If that seems like a sensible direction I'd be happy to work on a patch.
Thanks,
Lukas
--
Lukas Fittl
On 2022-Aug-09, Lukas Fittl wrote:
But I wonder, why do we have an explicit pretty printing flag on these
functions, and PRETTYFLAG_SCHEMA in the code to represent this behavior.
If we don't want pretty printing to affect schema qualification, why
does that flag exist?
Because of CVE-2018-1058. See commit 815172ba8068.
I imagine that that commit only touched the minimum necessary to solve
the immediate security problem, but that further work is needed to make
PRETTYFLAG_SCHEMA become a fully functional gadget; but that would
require that the whole of ruleutils.c (and everything downstream from
it) behaves sanely. In other words, I think your patch is too small.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
On 2022-Aug-09, Lukas Fittl wrote:
But I wonder, why do we have an explicit pretty printing flag on these
functions, and PRETTYFLAG_SCHEMA in the code to represent this behavior.
If we don't want pretty printing to affect schema qualification, why
does that flag exist?
Because of CVE-2018-1058. See commit 815172ba8068.
I imagine that that commit only touched the minimum necessary to solve
the immediate security problem, but that further work is needed to make
PRETTYFLAG_SCHEMA become a fully functional gadget; but that would
require that the whole of ruleutils.c (and everything downstream from
it) behaves sanely. In other words, I think your patch is too small.
What I'm inclined to do, rather than repeat the same finicky &
undocumented coding pattern in one more place, is write a convenience
function for it that can be named and documented to reflect the coding
rule about which call sites should use it (rather than calling plain
generate_relation_name). However, the first requirement for that
is to have a clearly defined rule. I think the intent of 815172ba8068
was to convert all uses that would determine the object-creation schema
in commands issued by pg_dump. Do we want to widen that, and if so
by how much? I'd be on board I think with adjusting other ruleutils.c
functions that could plausibly be used for building creation commands,
but happen not to be called by pg_dump. I'm not on board with
converting every single generate_relation_name call --- mainly because
it'd be pointless unless you also qualify every single function name,
operator name, etc; and that would be unreadable.
regards, tom lane
On Wed, Aug 10, 2022 at 09:48:08AM -0400, Tom Lane wrote:
What I'm inclined to do, rather than repeat the same finicky &
undocumented coding pattern in one more place, is write a convenience
function for it that can be named and documented to reflect the coding
rule about which call sites should use it (rather than calling plain
generate_relation_name). However, the first requirement for that
is to have a clearly defined rule. I think the intent of 815172ba8068
was to convert all uses that would determine the object-creation schema
in commands issued by pg_dump. Do we want to widen that, and if so
by how much? I'd be on board I think with adjusting other ruleutils.c
functions that could plausibly be used for building creation commands,
but happen not to be called by pg_dump. I'm not on board with
converting every single generate_relation_name call --- mainly because
it'd be pointless unless you also qualify every single function name,
operator name, etc; and that would be unreadable.
Lukas, please note that this patch is waiting for your input for a few
weeks now. Could you reply to the reviews provided?
--
Michael
On Wed, Oct 12, 2022 at 02:19:25PM +0900, Michael Paquier wrote:
Lukas, please note that this patch is waiting for your input for a few
weeks now. Could you reply to the reviews provided?
This has stalled for six weeks, so I have marked the patch as returned
with feedback.
--
Michael