Re: unlogged tables vs. GIST - Mailing list pgsql-hackers

On 23.01.2013 17:30, Robert Haas wrote:

On Wed, Jan 23, 2013 at 4:04 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com>  wrote:

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.

> ...

[a couple of good points]

In addition to those things Robert pointed out:

/*
+ * Temporary GiST indexes are not WAL-logged, but we need LSNs to detect
+ * concurrent page splits anyway. GetXLogRecPtrForUnloggedRel() provides a fake
+ * sequence of LSNs for that purpose. Each call generates an LSN that is
+ * greater than any previous value returned by this function in the same
+ * session using static counter
+ * Similarily unlogged GiST indexes are also not WAL-logged. But we need a
+ * persistent counter across clean shutdown. Use counter from ControlFile which
+ * is copied in XLogCtl.unloggedLSN to accomplish that
+ * If relation is UNLOGGED, return persistent counter from XLogCtl else return
+ * session wide temporary counter
+ */
+XLogRecPtr
+GetXLogRecPtrForUnloggedRel(Relation rel)

>From a modularity point of view, it's not good that you xlog.c needs to know about Relation struct. Perhaps move the logic to check the relation is unlogged or not to a function in gistutil.c, and only have a small GetUnloggedLSN() function in xlog.c

I'd suggest adding a separate spinlock to protect unloggedLSN. I'm not sure if there's much contention on XLogCtl->info_lck today, but nevertheless it'd be better to keep this separate, also from a modularity point of view.

@@ -7034,6 +7078,8 @@ CreateCheckPoint(int flags)
                LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
                ControlFile->state = DB_SHUTDOWNING;
                ControlFile->time = (pg_time_t) time(NULL);
+               /* Store unloggedLSN value as we want it persistent across shutdown */
+               ControlFile->unloggedLSN = XLogCtl->unloggedLSN;
                UpdateControlFile();
                LWLockRelease(ControlFileLock);
        }

This needs to acquire the spinlock to read unloggedLSN.

Do we need to do anything to unloggedLSN in pg_resetxlog?

- Heikki