Re: unlogged tables vs. GIST

Hi,

Any review comments on this ?

On Tue, Jan 29, 2013 at 6:03 PM, Jeevan Chalke <
jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:

> Hi Heikki,
>
>
> On Mon, Jan 28, 2013 at 2:34 PM, Heikki Linnakangas <
> hlinnakangas(at)vmware(dot)com> wrote:
>
>> On 23.01.2013 17:30, Robert Haas wrote:
>>
>>> On Wed, Jan 23, 2013 at 4:04 AM, Jeevan Chalke
>>> <jeevan(dot)chalke(at)enterprisedb(dot)**com <jeevan(dot)chalke(at)enterprisedb(dot)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 kept a function as is, but instead sending Relation as a parameter, it
> now takes boolean, isUnlogged. Added a MACRO for that.
>
>
>>
>> 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.
>>
>
> Hmm. OK.
> Added ulsn_lck for this.
>
>
>>
>> @@ -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.
>>
>
> OK. Done.
>
>
>>
>> Do we need to do anything to unloggedLSN in pg_resetxlog?
>>
>
> I guess NO.
>
> Also, added Robert's comment in bufmgr.c
> I am still unsure about the spinlock around buf flags as we are just
> examining it.
>
> Please let me know if I missed any.
>
> Thanks
>
>
>>
>> - Heikki
>>
>
>
>
> --
> Jeevan B Chalke
> Senior Software Engineer, R&D
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
> Phone: +91 20 30589500
>
> Website: www.enterprisedb.com
> EnterpriseDB Blog: http://blogs.enterprisedb.com/
> Follow us on Twitter: http://www.twitter.com/enterprisedb
>
> This e-mail message (and any attachment) is intended for the use of the
> individual or entity to whom it is addressed. This message contains
> information from EnterpriseDB Corporation that may be privileged,
> confidential, or exempt from disclosure under applicable law. If you are
> not the intended recipient or authorized to receive this for the intended
> recipient, any use, dissemination, distribution, retention, archiving, or
> copying of this communication is strictly prohibited. If you have received
> this e-mail in error, please notify the sender immediately by reply e-mail
> and delete this message.
>

--
Jeevan B Chalke
Senior Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. If you are
not the intended recipient or authorized to receive this for the intended
recipient, any use, dissemination, distribution, retention, archiving, or
copying of this communication is strictly prohibited. If you have received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.