Re: Parallel safety of binary_upgrade_create_empty_extension

On Mon, Mar 26, 2018 at 7:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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).

I'm not sure that deliberately mismarking things in the hopes that it
will make blind cut-and-paste a sensible approach is a very good
strategy.

> 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?

The only problem with running those in a worker (that I know about) is
that any locks they acquire wouldn't be retained.  But that is
technically a difference in semantics.

> 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'?

Same reason as above -- they acquire a lock on a named object and that
lock won't be retained if it happens in a worker.

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.

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

Yes.

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