On Wed, Mar 2, 2022 at 4:18 PM Andres Freund <and...@anarazel.de> wrote:
> I think there's just no way that it can be merged with anything close to the
> current design - it's unmaintainable. The need for the feature doesn't change
> that.
I don't know whether the design is right or wrong, but I agree that a
bad design isn't OK just because we need the feature. I'm not entirely
convinced that the change to _bt_getrootheight() is a red flag,
although I agree that there is a need to explain and justify why
similar changes aren't needed in other places. But I think overall
this patch is just too big and too unpolished to be seriously
considered. It clearly needs to be broken down into incremental
patches that are not just separated by topic but potentially
independently committable, with proposed commit messages for each.
And, like, there's a long history on this thread of people pointing
out particular crash bugs and particular problems with code comments
or whatever and I guess those are getting fixed as they are reported,
but I do not have the feeling that the overall code quality is
terribly high, because people just keep finding more stuff. Like look
at this:
+ uint8 flags = 0;
+
+ /* return 0 if feature is disabled */
+ if (max_active_gtt <= 0)
+ return InvalidTransactionId;
+
+ /* Disable in standby node */
+ if (RecoveryInProgress())
+ return InvalidTransactionId;
+
+ flags |= PROC_IS_AUTOVACUUM;
+ flags |= PROC_IN_LOGICAL_DECODING;
+
+ LWLockAcquire(ProcArrayLock, LW_SHARED);
+ arrayP = procArray;
+ for (index = 0; index < arrayP->numProcs; index++)
+ {
+ int pgprocno = arrayP->pgprocnos[index];
+ PGPROC *proc = &allProcs[pgprocno];
+ uint8 statusFlags = ProcGlobal->statusFlags[index];
+ TransactionId gtt_frozenxid = InvalidTransactionId;
+
+ if (statusFlags & flags)
+ continue;
This looks like code someone wrote, modified multiple times as they
found problems, and never cleaned up. 'flags' gets set to 0, and then
unconditionally gets two bits xor'd in, and then we test it against
statusFlags. Probably there shouldn't be a local variable at all, and
if there is, the value should be set properly from the start instead
of constructed incrementally as we go along. And there should be
comments. Why is it OK to return InvalidTransactionId in standby mode?
Why is it OK to pass that flags value? And, if we look at this
function a little further down, is it really OK to hold ProcArrayLock
across an operation that could perform multiple memory allocation
operations? I bet it's not, unless calls are very infrequent in
practice.
I'm not asking for this particular part of the code to be cleaned up.
I'm asking for the whole patch to be cleaned up. Like, nobody who is a
committer is going to have enough time to go through the patch
function by function and point out issues on this level of detail in
every place where they occur. Worse, discussing all of those issues is
just a distraction from the real task of figuring out whether the
design needs adjustment. Because the patch is one massive code drop,
and with not-really-that-clean code and not-that-great comments, it's
almost impossible to review. I don't plan to try unless the quality
improves a lot. I'm not saying it's the worst code ever written, but I
think it's kind of at a level of "well, it seems to work for me," and
the standard around here is higher than that. It's not the job of the
community or of individual committers to prove that problems exist in
this patch and therefore it shouldn't be committed. It's the job of
the author to prove that there aren't and it should be. And I don't
think we're close to that at all.
--
Robert Haas
EDB: http://www.enterprisedb.com
