On Sat, Dec 31, 2016 at 5:53 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Dec 30, 2016 at 10:59 PM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>>>
>>> Well, flushing the meta-data of pg_twophase is really going to be far
>>> cheaper than the many pages done until CheckpointTwoPhase is reached.
>>> There should really be a check on serialized_xacts for the
>>> non-recovery code path, but considering how cheap that's going to be
>>> compared to the rest of the restart point stuff it is not worth the
>>> complexity of adding a counter, for example in shared memory with
>>> XLogCtl (the counter gets reinitialized at each checkpoint,
>>> incremented when replaying a 2PC prepare, decremented with a 2PC
>>> commit). So to reduce the backpatch impact I would just do the fsync
>>> if (serialized_xact > 0 || RecoveryInProgress()) and call it a day.
>>
>> Sounds ok to me. I will leave it to the committer to decide further.
>
> Attached is an updated patch. I also noticed that it is better to do
> the fsync call before the dtrace call for checkpoint end as well as
> logging.
The patch looks good to me.
I am wondering what happens if a 2PC file gets created, at the time of
checkpoint we flush the pg_twophase directory, then the file gets
removed. Do we need to flush the directory to ensure that the removal
persists? Whatever material I look for fsync() on directory, it gives
examples of file creation, not that of deleting a file. If we want to
persist the removal, probably we have to flush pg_twophase always or
add code to track whether any activity happened in pg_twophase between
two checkpoints. The later seems complication not worth the benefit.
I guess, it's hard to construct a case to reproduce the issue
described in your first mail. But still checking if you have any
reproduction. May be we could use similar reproduction to test the
deletion of two phase file.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
