Re: Transaction Guarantee, updated version - Mailing list pgsql-patches

On Thu, 2007-06-21 at 16:05 -0400, Tom Lane wrote:
> "Simon Riggs" <simon@2ndquadrant.com> writes:
> > Completed all of the agreed changes for TG:
>
> I've just realized that there's a fatal problem with this design.
> We've now got tqual.c setting page LSN when it holds only share lock
> on the buffer.  That will absolutely not work, eg two backends might
> concurrently set different values and end up with garbage (since it's
> unlikely that LSN store is atomic).
>
> Can we fix it to be a read test instead of a write test, that is, if
> we know WAL has been flushed through the target LSN, it's safe to set
> the hint bit, else not?

Yes, that's roughly how it worked in v1.

The various possible strategies for handling hint bits were:
1. flush WAL to current insert pointer - very heavy perf hit
2. defer setting the hint bits at all, if the written transactions are
recently written. To do this we need a cache of recently written
deferred commit Xids.
3. flush WAL up to the LSN of the Xid, if it is a deferred commit. To do
this we need a cache of recently written deferred commit Xids.

Any more?

v1 implemented (2) on the basis that it was a small timing hole that was
best handled by doing as little as possible, though we could easily
implement (3) instead. But you need the deferred commit cache either
way, AFAICS.

So proposal is: Maintain cache of unflushed deferred commit xacts and
their corresponding LSNs. When you need to set a commit bit you check
the required LSN for the xact from cache, then flush WAL up to that LSN.
If xact doesn't exist, then do nothing because it is either an already
flushed deferred commit or a normal commit (therefore already flushed
too). Don't update the page's LSN at all. More code, but it works
correctly and efficiently.

This is pretty much putting back most of v1, though.

> In general, I think a transaction abort should not need to flush
> anything, since the default assumption is that it crashed anyway.

It currently does that though and I haven't sought to change that.

> Hence for instance recording a transaction abort needn't advance
> the LSN of the clog page.  (You seem to have it flushing through
> the last xlog record written by the backend, which is exactly what
> it doesn't need to do.)  By extension, it should be OK to set INVALID
> (aborted) hint bits in a tuple header without any concerns about
> flushing.

Sure.

> Also, I'm sort of wondering if we really need a separate walwriter
> process; that code seems awfully duplicative.  Is there a reason
> not to have the bgwriter include this functionality?

The bgwriter is doing something else already. Fsyncing the WAL takes
significant time and that would detract from the main role of bgwriter.

> In lesser news:
>
> The caching logic in TransactionGetCommitLSN is obviously broken.

OK, but won't check because of what you've said at top of mail. No need
to fix that if the basic premise needs changing anyway.

> Is there really a use-case for adding a pgstat counter for "guaranteed"
> transactions?  That adds pgstat overhead, and bloats the patch
> noticeably, and I don't entirely see the value of it.

I'm easy on that: its easier to take code out than add it in. It
certainly helped to debug things.

> There's some padding junk inserted in XLogCtlData, which as far as I
> recall was never discussed, and is certainly not an integral part of the
> delayed-commit feature.  If you want that you should submit and defend
> it separately.

OK

--
  Simon Riggs
  EnterpriseDB   http://www.enterprisedb.com