Re: unlogged tables vs. GIST

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