Обсуждение: Parallel query behaving different with custom GUCs
Hi All,
We observed the behavioral difference when query(with custom GUC) using
the PARALLEL plan vs the Non-PARALLEL plan.
Consider the below test:
I understand the given testcase doesn't make much sense, but this is the simplest
the PARALLEL plan vs the Non-PARALLEL plan.
Consider the below test:
I understand the given testcase doesn't make much sense, but this is the simplest
version of the test - to demonstrate the problem.
create table ptest2(id bigint, tenant_id bigint);
insert into ptest2 select g, mod(g,10) from generate_series(1, 1000000) g;
analyze ptest2;
-- Run the query by forcing the parallel plan.
postgres=> set max_parallel_workers_per_gather to 2;
SET
-- Error expected as custom GUC not set yet.
postgres=> select count(*) from ptest2 where current_setting('myapp.blah') is null;
ERROR: unrecognized configuration parameter "myapp.blah"
-- Set the customer GUC and execute the query.
postgres=> set myapp.blah to 999;
SET
postgres=> select count(*) from ptest2 where current_setting('myapp.blah') is null;
count
-------
0
(1 row)
-- RESET the custom GUC and rerun the query.
postgres=> reset myapp.blah;
RESET
-- Query should still run, but with forcing parallel plan, throwing an error.
postgres=> select count(*) from ptest2 where current_setting('myapp.blah') is null;
ERROR: unrecognized configuration parameter "myapp.blah"
CONTEXT: parallel worker
-- Disable the parallel plan and query just runs fine.
postgres=#set max_parallel_workers_per_gather to 0;
SET
postgres=#select count(*) from ptest2 where current_setting('myapp.blah') is null;
count
-------
0
(1 row)
Looking at the code, while serializing GUC settings function SerializeGUCState()
comments says that "We need only consider GUCs with source not PGC_S_DEFAULT".
Because of this when custom GUC is SET, it's an entry there in the "guc_nondef_list",
but when it's RESET, that is not more into "guc_nondef_list" and worker
is unable to access the custom GUC and ends up with the unrecognized parameter.
We might need another placeholder for the custom GUCs. Currently, we are
maintaining 3 linked lists in guc.c - guc_nondef_list, guc_stack_list,
guc_report_list and to fix the above issue either we need a 4th list or do
changes in the existing list.
Thought/Comments?
Regards,
create table ptest2(id bigint, tenant_id bigint);
insert into ptest2 select g, mod(g,10) from generate_series(1, 1000000) g;
analyze ptest2;
-- Run the query by forcing the parallel plan.
postgres=> set max_parallel_workers_per_gather to 2;
SET
-- Error expected as custom GUC not set yet.
postgres=> select count(*) from ptest2 where current_setting('myapp.blah') is null;
ERROR: unrecognized configuration parameter "myapp.blah"
-- Set the customer GUC and execute the query.
postgres=> set myapp.blah to 999;
SET
postgres=> select count(*) from ptest2 where current_setting('myapp.blah') is null;
count
-------
0
(1 row)
-- RESET the custom GUC and rerun the query.
postgres=> reset myapp.blah;
RESET
-- Query should still run, but with forcing parallel plan, throwing an error.
postgres=> select count(*) from ptest2 where current_setting('myapp.blah') is null;
ERROR: unrecognized configuration parameter "myapp.blah"
CONTEXT: parallel worker
-- Disable the parallel plan and query just runs fine.
postgres=#set max_parallel_workers_per_gather to 0;
SET
postgres=#select count(*) from ptest2 where current_setting('myapp.blah') is null;
count
-------
0
(1 row)
Looking at the code, while serializing GUC settings function SerializeGUCState()
comments says that "We need only consider GUCs with source not PGC_S_DEFAULT".
Because of this when custom GUC is SET, it's an entry there in the "guc_nondef_list",
but when it's RESET, that is not more into "guc_nondef_list" and worker
is unable to access the custom GUC and ends up with the unrecognized parameter.
We might need another placeholder for the custom GUCs. Currently, we are
maintaining 3 linked lists in guc.c - guc_nondef_list, guc_stack_list,
guc_report_list and to fix the above issue either we need a 4th list or do
changes in the existing list.
Thought/Comments?
Regards,
Rushabh Lathia
On Thu, Oct 26, 2023 at 3:10 AM Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
> -- RESET the custom GUC and rerun the query.
> postgres=> reset myapp.blah;
> RESET
>
> -- Query should still run, but with forcing parallel plan, throwing an error.
> postgres=> select count(*) from ptest2 where current_setting('myapp.blah') is null;
> ERROR: unrecognized configuration parameter "myapp.blah"
> CONTEXT: parallel worker
>
> -- Disable the parallel plan and query just runs fine.
> postgres=#set max_parallel_workers_per_gather to 0;
> SET
> postgres=#select count(*) from ptest2 where current_setting('myapp.blah') is null;
> count
> -------
> 0
> (1 row)
>
> We might need another placeholder for the custom GUCs. Currently, we are
> maintaining 3 linked lists in guc.c - guc_nondef_list, guc_stack_list,
> guc_report_list and to fix the above issue either we need a 4th list or do
> changes in the existing list.
I discussed this a bit with Rushabh off-list before he posted, and was
hoping someone else would reply, but since no one has:
Formally, I think this is a bug. However, the practical impact of it
is fairly low, because you have to be using custom GUCs in your query
and you have to RESET them instead of using SET to put them back to
the default value, which I'm guessing is something that not a lot of
people do. I'm a bit concerned that adding the necessary tracking
could be expensive, and I'm not sure we want to slow down things in
normal cases to cater to this somewhat strange case. On the other
hand, maybe we can fix it without too much cost, in which case that
would be good to do.
I'm also alert to my own possible bias. Perhaps since I designed this
mechanism, I'm more prone to viewing its deficiencies as minor than a
neutral observer would be. So if anyone is sitting there reading this
and thinking "wow, I can't believe Robert doesn't think it's important
to fix this," feel free to write back and say so.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Mon, Oct 30, 2023 at 09:21:56AM -0400, Robert Haas wrote: > I'm also alert to my own possible bias. Perhaps since I designed this > mechanism, I'm more prone to viewing its deficiencies as minor than a > neutral observer would be. So if anyone is sitting there reading this > and thinking "wow, I can't believe Robert doesn't think it's important > to fix this," feel free to write back and say so. Fun. Agreed that this is a bug, and that the consequences are of null for most users. And it took 7 years to find that. If I may ask, is there an impact with functions that include SET clauses with custom parameters that are parallel safe? Saying that, if the fix is simple, I see no reason not to do something about it, even if that's HEAD-only. -- Michael