Re: unlogged tables vs. GIST

On Wed, Jan 23, 2013 at 4:04 AM, Jeevan Chalke
<jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
> Yes.
>
> I guess my earlier patch, which was directly incrementing
> ControlFile->unloggedLSN counter was the concern as it will take
> ControlFileLock several times.
>
> In this version of patch I did what Robert has suggested. At start of the
> postmaster, copying unloggedLSn value to XLogCtl, a shared memory struct.
> And
> in all access to unloggedLSN, using this shared variable using a SpinLock.
> And since we want to keep this counter persistent across clean shutdown,
> storing it in ControlFile before updating it.
>
> With this approach, we are updating ControlFile only when we shutdown the
> server, rest of the time we are having a shared memory counter. That means
> we
> are not touching pg_control every other millisecond or so. Also since we are
> not caring about crashes, XLogging this counter like OID counter is not
> required as such.

On a quick read-through this looks reasonable to me, but others may
have different opinions, and I haven't reviewed in detail.

One obvious oversight is that bufmgr.c needs a detailed comment
explaining the reason behind the change you are making there.
Otherwise, someone might think to undo it in the future, and that
would be bad. Perhaps add something like:

However, this rule does not apply for unlogged relations, which will
be lost after a crash anyway. Most unlogged relation pages do not
bear LSNs since we never emit WAL records for them, and therefore
flushing up through the buffer LSN would be useless, but harmless.
However, GiST indexes use LSNs internally to track page-splits, and
therefore temporary and unlogged GiST pages bear "fake" LSNs generated
by GetXLogRecPtrForUnloggedRel. It is unlikely but possible that the
fake-LSN counter used for unlogged relations could advance past the
WAL insertion point; and if it did happen, attempting to flush WAL
through that location would fail, with disastrous system-wide
consequences. To make sure that can't happen, skip the flush if the
buffer isn't permanent.

A related problem is that you're examining the buffer header flags
without taking the buffer-header spinlock. I'm not sure this can
actually matter, because we'll always hold the content lock on the
buffer at this point, which presumably precludes any operation that
might flip that bit, but it looks like the callers all have the buffer
header flags conveniently available, so maybe they should pass that
information down to FlushBuffer. That would also avoid accessing that
word in memory both before and after releasing the spinlock (all
callers have just released it) which can lead to memory-access stalls.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company