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
