On Thu, Nov 02, 2017 at 04:20:19PM -0400, Peter Eisentraut wrote: > I haven't really thought about this feature too hard; I just want to > give you a couple of code comments.
Thanks! > I think the catalog structure, and relatedly also the parser structures, > could be made more compact. We currently have condeferrable and > condeferred to represent three valid states (NOT DEFERRABLE, DEFERRABLE > INITIALLY IMMEDIATE, DEFERRABLE INITIALLY DEFERRED). You are adding > conalwaysdeferred, but you are adding only additional state (ALWAYS > DEFERRED). So we end up with three bool fields to represent four > states. I think this should all be rolled into one char field with four > states. I thought about this. I couldn't see a way to make the two existing boolean columns have a combination meaning "ALWAYS DEFERRED" that might not break something else. Since (condeferred AND NOT condeferrable) is an impossible combination today, I could use it to mean ALWAYS DEFERRED. I'm not sure how safe that would be. And it does seem like a weird way to express ALWAYS DEFERRED, though it would work. Replacing condeferred and condeferrable with a char columns also occurred to me, and though I assume backwards-incompatible changes to pg_catalog tables are fair game, I assumed everyone would prefer avoiding such changes where possible. Also, a backwards-incompatible change to the table would significantly enlarge the patch, as more version checks would be needed, particularly regarding upgrades (which are otherwise trivial). I felt adding a new column was probably safest. I'll make a backwards- incompatible change if requested, naturally, but I guess I'd want to get wider consensus on that, as I fear others may not agree. That fear may just be due to my ignorance of the community's preference as to pg_catalog backwards-compatibility vs. cleanliness. Hmmm, must I do anything special about _downgrades_? Does PostgreSQL support downgrades? > In psql and pg_dump, if you are query new catalog fields, you need to > have a version check to have a different query for >=PG11. (This would > likely apply whether you adopt my suggestion above or not.) Ah, yes, of course. I will add such a check. > Maybe a test case in pg_dump would be useful. Will do. > Other than that, this looks like a pretty complete patch. Thanks for the review! It's a small-ish patch, and my very first for PG. It was fun writing it. I greatly appreciate that PG source is easy to read. Nico -- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
