Re: Parallel safety of binary_upgrade_create_empty_extension

On Tue, Mar 27, 2018 at 11:17 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> So, your proposal is to do nothing and just hope we don't make the same
> mistake again in future?

That wasn't really my proposal, but if it were, it would be at least
as good as your proposal of making changing things in an unprincipled
fashion and hoping we get the right result. :-)

I think what we need to do is write either a README or some SGML
documentation explaining the reasoning behind existing markings with
an eye toward helping future patch authors/committers get this right.
I'm willing to do that work, or at least a first draft of it, although
not today.

> Meh. It's not documented that pg_get_viewdef takes any locks, and
> I'm sure most users would be astonished to learn that, and this
> argument surely doesn't explain why pg_get_viewdef is restricted
> while pg_get_ruledef is marked safe.

I'm personally of the opinion that we'd be smarter to make some things
that currently hold locks until commit release them immediately, which
would then make it reasonable to mark such things parallel-safe. I
don't think it makes much sense to keep the locks only sometimes and
pretend like the difference doesn't matter, though. That's telling
people "it's very important to hold these locks until commit, except
when it's not!" which doesn't seem like it can be right.

>> Regarding cursor_to_xml, I don't think the problem you mention exists,
>> because the query the cursor runs is independently subject to the
>> parallel restrictions.
>
> Well, "independently" is exactly the point. If the cursor query contains
> some parallel-unsafe (not just parallel-restricted) operations, and we
> allow cursor_to_xml to be parallel-restricted, don't we end up risking
> executing parallel-unsafe operations while doing parallelism?
>
> Or to be concrete:
>
> regression=# create sequence s1;
> CREATE SEQUENCE
> regression=# begin;
> BEGIN
> regression=# set force_parallel_mode to 1;
> SET
> regression=# declare c cursor for select nextval('s1');
> DECLARE CURSOR
> regression=# select cursor_to_xml('c',1,true,true,'');
> ERROR: cannot execute nextval() during a parallel operation
>
> I think this behavior is a bug.

I agree.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company