Re: [HACKERS] Fix mdsync never-ending loop problem

Tom Lane wrote:
> Heikki Linnakangas <heikki@enterprisedb.com> writes:
>> My first thought is that the cycle_ctr just adds extra complexity. The
>> canceled-flag really is the key in Takahiro-san's patch, so we don't
>> need the cycle_ctr anymore.
>
> We don't have to have it in the sense of the code not working without it,
> but it probably pays for itself by eliminating useless fsyncs.  The
> overhead for it in my proposed implementation is darn near zero in the
> non-error case.  Also, Takahiro-san mentioned at one point that he was
> concerned to avoid useless fsyncs because of some property of the LDC
> patch --- I wasn't too clear on what, but maybe he can explain.

Ok. Perhaps we should not use the canceled-flag but just remove the
entry from pendingOpsTable like we used to when mdsync_in_progress isn't
set. We might otherwise accumulate a lot of canceled entries in the hash
table if checkpoint interval is long and relations are created and
dropped as part of normal operation.

I think there's one little bug in the patch:

1. AbsorbFsyncRequests is called. A FORGET message is received, and an
entry in the hash table is marked as canceled
2. Another relation with the same relfilenode is created. This can
happen after OID wrap-around
3. RememberFsyncRequest is called for the new relation. The old entry is
still in the hash table, marked with the canceled-flag, so it's not touched.

The fsync request for the new relation is masked by the old canceled
entry. The trivial fix is to always clear the flag on step 3:

--- md.c        2007-04-11 08:18:08.000000000 +0100
+++ md.c.new    2007-04-12 09:21:00.000000000 +0100
@@ -1161,9 +1161,9 @@
  &found);
                 if (!found)                             /* new entry,
so initialize it */
                 {
-                       entry->canceled = false;
                         entry->cycle_ctr = mdsync_cycle_ctr;
                 }
+               entry->canceled = false;
                 /*
                  * NB: it's intentional that we don't change cycle_ctr
if the entry
                  * already exists.  The fsync request must be treated
as old, even


--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com