Re: Parallel safety of binary_upgrade_create_empty_extension

Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> writes:
> On Tue, Mar 27, 2018 at 11:58 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I wonder whether we shouldn't mark *all* of these parallel-unsafe.
>> I'm not exactly convinced that 'restricted' is sufficient for the
>> others, and even if it is, there's certainly little if any upside
>> for letting them be executed in parallel-enabled mode.

> Yeah, I almost sent a patch to do exactly that, but then I realised
> the others all just set a global variable so they're technically fine
> as 'r'.

Yeah, so I see after looking closer.

> I have no strong preference either way; these functions will
> only actually be run in parallel in the weird situation of
> force_parallel_mode = on.

While the minimal patch would be fine for now, what I'm worried about
is preventing future mistakes. It seems highly likely that the reason
binary_upgrade_create_empty_extension is marked 'r' is that somebody
copied-and-pasted that from another binary_upgrade_foo function without
thinking very hard. Marking them all 'u' would help to forestall future
mistakes of the same sort, while leaving them as 'r' doesn't seem to buy
us anything much (beyond a smaller catalog patch today).

Querying for other functions marked 'r' leaves me with some other related
doubts:

1. Why are the various flavors of pg_get_viewdef() marked 'r'? Surely
reading the catalogs is a thing parallel children are allowed to do.
If there is a good reason for pg_get_viewdef() to be 'r', why doesn't
the same reason apply to all the other ruleutils functions?

2. Why are the various thingy-to-xml functions marked 'r'? Again it
can't be because they read catalogs or data. I can imagine that the
reason for restricting cursor_to_xml is that the cursor might execute
parallel-unsafe operations, but then why isn't it marked 'u'?

3. Isn't pg_import_system_collations() unsafe, for the same reason
as binary_upgrade_create_empty_extension()?

regards, tom lane