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
