On 2020-Oct-22, Tom Lane wrote: > 1. I think this bit in LockViewRecurse_walker must be removed as well: > > /* Currently, we only allow plain tables or views to be locked. */ > if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE && > relkind != RELKIND_VIEW) > continue; > > (Mostly, stuff in views would be ordinary tables anyway ... but > foreign tables are a possible exception.) Actually, foreign tables are not a problem: I guess they were hammered out enough when somebody allowed them to be partitions, for partitioned table recursive locking. What *is* a problem with that change is any RTE that's not RTE_RELATION. It's not obvious in the above test, but the most important case being left out is "relkind == '\0'" which is what you get for other RTEs; so that broke when removing the above. So I added a "continue" for other RTE kinds, and now it all seems to work correctly as far as I can tell. While on this: it's important to note that lack of recursion here to join RTEs does not break anything, since the relations being joined are separately part of the rtable so they'll be processed anyway. BTW I don't know what's the possible usefulness of locking standalone composite types, but it's there. > 2. Should we add anything to the LOCK TABLE reference page? I see nothing > there now that bears on this change, but ... Eh. There's a couple of places where it now seems more correct to talk about a *relation* being locked. I went as far as mentioning the set of relkinds, for extra clarity (I skipped toast tables because it seemed pointless). The subject of the ONLY keyword not applying to views was a bit surprising so I added that too. Not 100% wedded on the wording of these changes. > 3. Is hard-coding #define's for ERRCODEs actually the best we can do here? > I guess it is right now, but ugh. Seems like we ought to find a way for > frontend code to make use of errcodes.h. Not a matter for this patch > though. Yeah, I had the same though -- didn't like this at all but evidently it hasn't bothered anyone sufficiently. > 4. You neglected to change the table name in the warn_or_exit_horribly > error message. Ugh, fixed. > There might be some value in using one of pg_class's indexes instead, > mainly because the parser will surely need to touch that on the way > to performing the LOCK, while pg_aggregate is far afield. Hmm. I ended up using pg_class_tblspc_relfilenode_index, which I suppose would be the least trafficked index of pg_class. At least, holding that one doesn't block other connections (so I could use LOCK TABLE <that index>; in another session and still launch pg_dump to verify it worked correctly)
