On Mon, Mar 26, 2018 at 7:23 PM, Tom Lane <t...@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
