Обсуждение: 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));
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)
-------------------------------------------------------------------------------------------------------
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
Вложения
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
Re: pg_get_constraintdef: Schema qualify foreign tables unless pretty printing is enabled
От
Alvaro Herrera
Дата:
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
Re: pg_get_constraintdef: Schema qualify foreign tables unless pretty printing is enabled
От
Michael Paquier
Дата:
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
Вложения
Re: pg_get_constraintdef: Schema qualify foreign tables unless pretty printing is enabled
От
Michael Paquier
Дата:
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