Обсуждение: Re: [PATCHES] Fix mdsync never-ending loop problem

Heikki Linnakangas <heikki@enterprisedb.com> wrote:
> Itagaki, would you like to take a stab at this?

Yes, I'll try to fix the mdsync problem. I'll separate this fix from LDC
patch. If we need to backport the fix to the back branches, a stand-alone
patch would be better.

In my understanding from the discussion, we'd better to take "cycle ID"
approach instead of "making a copy of pendingOpsTable", because duplicated
table is hard to debug and requires us to pay attention not to leak memories.
I'll adopt the cycle ID approach and build LDC on it as a separate patch.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
> In my understanding from the discussion, we'd better to take "cycle ID"
> approach instead of "making a copy of pendingOpsTable", because duplicated
> table is hard to debug and requires us to pay attention not to leak memories.
> I'll adopt the cycle ID approach and build LDC on it as a separate patch.

Heikki made some reasonable arguments against the cycle-ID idea.  I'm
not intending to insist on it ...

I do think there are multiple issues here and it'd be better to try
to separate the fixes into different patches.
        regards, tom lane

I wrote:
> This patch looks fairly sane to me; I have a few small gripes about
> coding style but that can be fixed while applying.  Heikki, you were
> concerned about the cycle-ID idea; do you have any objection to this
> patch?

Actually, on second look I think the key idea here is Takahiro-san's
introduction of a cancellation flag in the hashtable entries, to
replace the cases where AbsorbFsyncRequests can try to delete entries.

What that means is mdsync() doesn't need an outer retry loop at all:
the periodic AbsorbFsyncRequests calls are not a hazard, and retry of
FileSync failures can be handled as an inner loop on the single failing
table entry.  (We can make the failure counter a local variable, too,
instead of needing space in every hashtable entry.)

And with that change, it's no longer possible for an incoming stream
of fsync requests to keep mdsync from terminating.  It might fsync
more than it really needs to, but it won't repeat itself, and it must
reach the end of the hashtable eventually.  So we don't actually need
the cycle counter at all.

It might be worth having the cycle counter anyway just to avoid doing
"useless" fsync work.  I'm not sure about this.  If we have a cycle
counter of say 32 bits, then it's theoretically possible for an fsync
to fail 2^32 consecutive times and then be skipped on the next try,
allowing a checkpoint to succeed that should not have.  We can fix that
with a few more lines of logic to detect a wrapped-around value, but is
it worth the trouble?

            regards, tom lane

(Sorry if you receive duplicate messages. I resend it since it was not
 delivered after a day.)


Here is another patch to fix never-ending loop in mdsync. I introduced
a mdsync counter (cycle id) and cancel flags to fix the problem.

The mdsync counter is incremented at the every beginning of mdsync().
Each pending entry has a field assigned from the counter when it is
newly inserted to pendingOpsTable. Only entries that have smaller counter
values than the mdsync counter are fsync-ed in mdsync().

Another change is to add a cancel flag in each pending entry. When a
relation is dropped and bgwriter receives a forget-request, the corresponding
entry is marked as dropped but we don't delete it at that time. Actual
deletion is performed in the next fsync loop. We don't have to retry after
AbsorbFsyncRequests() because entries are not removed outside of seqscan.

This patch can be applied to HEAD, 8.2 and 8.1 with a few hunks.


Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > In my understanding from the discussion, we'd better to take "cycle ID"
> > approach instead of "making a copy of pendingOpsTable", because duplicated
> > table is hard to debug and requires us to pay attention not to leak memories.
> > I'll adopt the cycle ID approach and build LDC on it as a separate patch.
>
> Heikki made some reasonable arguments against the cycle-ID idea.  I'm
> not intending to insist on it ...

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
> Here is another patch to fix never-ending loop in mdsync. I introduced
> a mdsync counter (cycle id) and cancel flags to fix the problem.

> The mdsync counter is incremented at the every beginning of mdsync().
> Each pending entry has a field assigned from the counter when it is
> newly inserted to pendingOpsTable. Only entries that have smaller counter
> values than the mdsync counter are fsync-ed in mdsync().

> Another change is to add a cancel flag in each pending entry. When a
> relation is dropped and bgwriter receives a forget-request, the corresponding
> entry is marked as dropped but we don't delete it at that time. Actual
> deletion is performed in the next fsync loop. We don't have to retry after
> AbsorbFsyncRequests() because entries are not removed outside of seqscan.

This patch looks fairly sane to me; I have a few small gripes about
coding style but that can be fixed while applying.  Heikki, you were
concerned about the cycle-ID idea; do you have any objection to this
patch?

> This patch can be applied to HEAD, 8.2 and 8.1 with a few hunks.

I don't think we should back-patch something that's a performance fix
for an extreme case, especially not when it's not been through any
extensive testing yet ...
        regards, tom lane