Re: [HACKERS] unlogged tables vs. GIST
Hi Heikki, On Mon, Feb 11, 2013 at 7:28 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 11.02.2013 08:44, Jeevan Chalke wrote: Hi, Any review comments on this ? Sorry for the delay. I did some minor cleanup on this. I added code to pg_resetxlog and pg_controldata to reset / display the current unlogged LSN value. I moved the static counter, for temporary relations, back to gistutil.c, so that the function in xlog.c only deals with unlogged relations. It's debatable if that's better, but IMHO it is. Also, the unloggedLSN counter is now reset to 1 at crash recovery. There's no fundamental reason it needs to be reset, rather than just continue from the last shutdowned value like nothing happened, but it seems cleaner that way. I'm happy with this now, but please take one more look before I commit this. This morning I had a look over this. But it seems that you have already committed it. Changes are fine and even better. No issues with my unit testing too. Thanks for the commit. - Heikki -- Jeevan B Chalke Senior Software Engineer, RD 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.
Re: [HACKERS] unlogged tables vs. GIST
On 11.02.2013 08:44, Jeevan Chalke wrote:
Hi,
Any review comments on this ?
Sorry for the delay.
I did some minor cleanup on this. I added code to pg_resetxlog and
pg_controldata to reset / display the current unlogged LSN value. I
moved the static counter, for temporary relations, back to gistutil.c,
so that the function in xlog.c only deals with unlogged relations. It's
debatable if that's better, but IMHO it is. Also, the unloggedLSN
counter is now reset to 1 at crash recovery. There's no fundamental
reason it needs to be reset, rather than just continue from the last
shutdowned value like nothing happened, but it seems cleaner that way.
I'm happy with this now, but please take one more look before I commit this.
- Heikki
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 8872920..af11eb0 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -182,8 +182,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
automatically truncated after a crash or unclean shutdown. The contents
of an unlogged table are also not replicated to standby servers.
Any indexes created on an unlogged table are automatically unlogged as
- well; however, unlogged link linkend=GiSTGiST indexes/link are
- currently not supported and cannot be created on an unlogged table.
+ well.
/para
/listitem
/varlistentry
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index e2d3390..eba95f1 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -16,6 +16,7 @@
#include access/genam.h
#include access/gist_private.h
+#include access/heapam_xlog.h
#include catalog/index.h
#include catalog/pg_collation.h
#include miscadmin.h
@@ -71,9 +72,22 @@ createTempGistContext(void)
Datum
gistbuildempty(PG_FUNCTION_ARGS)
{
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg(unlogged GiST indexes are not supported)));
+ Relation index = (Relation) PG_GETARG_POINTER(0);
+ Buffer buffer;
+
+ /* Initialize the root page */
+ buffer = ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);
+ LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
+
+ /* Initialize and xlog buffer */
+ START_CRIT_SECTION();
+ GISTInitBuffer(buffer, F_LEAF);
+ MarkBufferDirty(buffer);
+ log_newpage_buffer(buffer);
+ END_CRIT_SECTION();
+
+ /* Unlock and release the buffer */
+ UnlockReleaseBuffer(buffer);
PG_RETURN_VOID();
}
@@ -391,7 +405,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
dist, oldrlink, oldnsn, leftchildbuf,
markfollowright);
else
- recptr = GetXLogRecPtrForTemp();
+ recptr = gistGetFakeLSN(rel);
for (ptr = dist; ptr; ptr = ptr-next)
{
@@ -448,7 +462,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
}
else
{
- recptr = GetXLogRecPtrForTemp();
+ recptr = gistGetFakeLSN(rel);
PageSetLSN(page, recptr);
}
diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c
index aec5b52..0cf22cd 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -158,16 +158,6 @@ gistbuild(PG_FUNCTION_ARGS)
elog(ERROR, index \%s\ already contains data,
RelationGetRelationName(index));
- /*
- * We can't yet handle unlogged GiST indexes, because we depend on LSNs.
- * This is duplicative of an error in gistbuildempty, but we want to check
- * here so as to throw error before doing all the index-build work.
- */
- if (heap-rd_rel-relpersistence == RELPERSISTENCE_UNLOGGED)
- ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg(unlogged GiST indexes are not supported)));
-
/* no locking is needed */
buildstate.giststate = initGISTstate(index);
@@ -204,7 +194,7 @@ gistbuild(PG_FUNCTION_ARGS)
PageSetTLI(page, ThisTimeLineID);
}
else
- PageSetLSN(page, GetXLogRecPtrForTemp());
+ PageSetLSN(page, gistGetFakeLSN(heap));
UnlockReleaseBuffer(buffer);
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index e5c3d69..c5b2c87 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -798,16 +798,30 @@ gistoptions(PG_FUNCTION_ARGS)
}
/*
- * Temporary GiST indexes are not WAL-logged, but we need LSNs to detect
- * concurrent page splits anyway. GetXLogRecPtrForTemp() 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.
+ * Temporary and unlogged GiST indexes are not WAL-logged, but we need LSNs
+ * to detect concurrent page splits anyway. This function provides a fake
+ * sequence of LSNs for that purpose.
*/
XLogRecPtr
-GetXLogRecPtrForTemp(void)
+gistGetFakeLSN(Relation rel)
{
static XLogRecPtr counter = 1;
-
Re: [HACKERS] unlogged tables vs. GIST
Hi, Any review comments on this ? On Tue, Jan 29, 2013 at 6:03 PM, Jeevan Chalke jeevan.cha...@enterprisedb.com wrote: Hi Heikki, On Mon, Jan 28, 2013 at 2:34 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 23.01.2013 17:30, Robert Haas wrote: On Wed, Jan 23, 2013 at 4:04 AM, Jeevan Chalke jeevan.chalke@enterprisedb.**com jeevan.cha...@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 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, RD 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, RD 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
Re: [HACKERS] unlogged tables vs. GIST
Hi Heikki, On Mon, Jan 28, 2013 at 2:34 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 23.01.2013 17:30, Robert Haas wrote: On Wed, Jan 23, 2013 at 4:04 AM, Jeevan Chalke jeevan.chalke@enterprisedb.**com jeevan.cha...@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 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, RD 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. support_unlogged_gist_indexes_v3.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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.cha...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On Mon, Jan 28, 2013 at 4:04 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: Do we need to do anything to unloggedLSN in pg_resetxlog? Does the server go into recovery after pg_resetxlog? If so, no. If not, probably, but I have no idea what. There's no safe value in that case; what we ought to do is force a reset of all unlogged relations, or just punt and hope nothing bad happens (which after all is what pg_resetxlog is mostly about anyway). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On Wed, Jan 16, 2013 at 3:24 AM, Robert Haas robertmh...@gmail.com wrote: On Tue, Jan 15, 2013 at 4:26 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: I think that might be acceptable from a performance point of view - after all, if the index is unlogged, you're saving the cost of WAL - but I guess I still prefer a generic solution to this problem (a generalization of GetXLogRecPtrForTemp) rather than a special-purpose solution based on the nitty-gritty of how GiST uses these values. What's the difference between storing this value in pg_control and, say, the OID counter? Well, the modularity argument is that GiST shouldn't have any special privileges compared to a third-party index AM. (I realize that third-party AMs already have problems plugging into WAL replay, but that doesn't mean we should create more problems.) We could possibly dodge that objection by regarding the global counter as some sort of generic unlogged operation counter, available to anybody who needs it. It would be good to have a plausible example of something else needing it, but assume somebody can think of one. The bigger issue is that the reason we don't have to update pg_control every other millisecond is that the OID counter is capable of tracking its state between checkpoints without touching pg_control, that is it can emit WAL records to track its increments. I think that we should insist that GiST do likewise, even if we give it some space in pg_control. Remember that pg_control is a single point of failure for the database, and the more often it's written to, the more likely it is that something will go wrong there. So I guess what would make sense to me is that we invent an unlogged ops counter that is managed exactly like the OID counter, including having WAL records that are treated as consuming some number of values in advance. If it's 64 bits wide then the WAL records could safely be made to consume quite a lot of values, like a thousand or so, thus reducing the actual WAL I/O burden to about nothing. I didn't look at the actual patch (silly me?) but the only time you need to update the control file is when writing the shutdown checkpoint just before stopping the database server. If the server crashes, it's OK to roll the value back to some smaller value, because unlogged relations will be reset anyway. And while the server is running the information can live in a shared memory copy protected by a spinlock. So the control file traffic should be limited to once per server lifetime, AFAICS. 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. Thanks -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Jeevan B Chalke Senior Software Engineer, RD 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. support_unlogged_gist_indexes_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On Wed, Jan 23, 2013 at 4:04 AM, Jeevan Chalke jeevan.cha...@enterprisedb.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On 15.01.2013 08:54, Jeevan Chalke wrote: For (2), I have added a new function called, GetXLogRecPtrForUnloogedRel() which returns a fake LSN for GiST indexes. However, I have removed GetXLogRecPtrForTemp() function and added its functionality inside this new function itself to avoid complexity. I don't much care for using a new field in the control file for this. First, it seems like a big modularity violation to store a gist-specific counter in the control file. Second, you'd be generating a lot of traffic on the ControlFileLock. It's not heavily contended at the moment, but when the control file is updated, it's held over an fsync, which could cause unnecessary stalls to insertions to unlogged gist tables. And it's just a bad idea to share a lock for two things with completely different characteristics in general. Could we stash the counter e.g. in the root page of the index? - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On Tue, Jan 15, 2013 at 1:10 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 15.01.2013 08:54, Jeevan Chalke wrote: For (2), I have added a new function called, GetXLogRecPtrForUnloogedRel() which returns a fake LSN for GiST indexes. However, I have removed GetXLogRecPtrForTemp() function and added its functionality inside this new function itself to avoid complexity. I don't much care for using a new field in the control file for this. First, it seems like a big modularity violation to store a gist-specific counter in the control file. Second, you'd be generating a lot of traffic on the ControlFileLock. It's not heavily contended at the moment, but when the control file is updated, it's held over an fsync, which could cause unnecessary stalls to insertions to unlogged gist tables. And it's just a bad idea to share a lock for two things with completely different characteristics in general. Could we stash the counter e.g. in the root page of the index? That would require maintaining a counter per table rather than a single global counter, which would be bad because then we'd need to store one counter in shared memory for every table, rather than just one, period, which runs up against the fixed sizing of shared memory. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On 15.01.2013 20:33, Robert Haas wrote:
On Tue, Jan 15, 2013 at 1:10 PM, Heikki Linnakangas
Could we stash the counter e.g. in the root page of the index?
That would require maintaining a counter per table rather than a
single global counter, which would be bad because then we'd need to
store one counter in shared memory for every table, rather than just
one, period, which runs up against the fixed sizing of shared memory.
I was thinking of just adding a new field to the root page header, and
use that field as the counter. Something like:
XLogRecPtr
GetXLogRecPtrForTemp(void)
{
rootbuf = ReadBuffer(rel, GIST_ROOT_BLKNO);
opaq = GistPageGetOpaque(BufferGetPage(rootbuf));
LockBuffer(rootbuf, GIST_EXCLUSIVE);
nsn = opaq-counter++
UnlockReleaseBuffer(rootbuf)
return nsn;
}
or perhaps we need to use locking mechanism for that, like just a new
global lwlock or spinlock, to avoid deadlocks if someone is just
splitting the root page. In any case, the fixed-sizedness of shared
memory isn't an issue here.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
Robert Haas robertmh...@gmail.com writes: On Tue, Jan 15, 2013 at 1:10 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: Could we stash the counter e.g. in the root page of the index? That would require maintaining a counter per table rather than a single global counter, which would be bad because then we'd need to store one counter in shared memory for every table, rather than just one, period, which runs up against the fixed sizing of shared memory. I think what Heikki had in mind was that the copy in the index would be the authoritative one, not some image in shared memory. This'd imply dirtying the root page on every insert, as well as increased contention for the root page, so it might have performance problems. I think a bigger issue is where we'd find any space for it. There's no easily-spare space in a GIST page. This reminds me again that the lack of a metapage in GIST was a serious design error, which we should correct if we ever break on-disk compatibility again. I concur that adding such a counter to pg_control is a nonstarter, though. Given that we don't need crash recovery for an unlogged table, could we get away with some variant of NSN that has weaker semantics than XLOG LSNs? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On Tue, Jan 15, 2013 at 1:48 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Jan 15, 2013 at 1:10 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: Could we stash the counter e.g. in the root page of the index? That would require maintaining a counter per table rather than a single global counter, which would be bad because then we'd need to store one counter in shared memory for every table, rather than just one, period, which runs up against the fixed sizing of shared memory. I think what Heikki had in mind was that the copy in the index would be the authoritative one, not some image in shared memory. This'd imply dirtying the root page on every insert, as well as increased contention for the root page, so it might have performance problems. I think a bigger issue is where we'd find any space for it. There's no easily-spare space in a GIST page. This reminds me again that the lack of a metapage in GIST was a serious design error, which we should correct if we ever break on-disk compatibility again. I concur that adding such a counter to pg_control is a nonstarter, though. Given that we don't need crash recovery for an unlogged table, could we get away with some variant of NSN that has weaker semantics than XLOG LSNs? It needs to be strictly ascending and survive clean shutdowns. Is there some place we could preserve it other than the control file? I was assuming we wanted a single sequence shared across all relations rather than a sequence per relation, but I don't know of any reason why that's actually required. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On 15.01.2013 20:48, Tom Lane wrote: Robert Haasrobertmh...@gmail.com writes: On Tue, Jan 15, 2013 at 1:10 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: Could we stash the counter e.g. in the root page of the index? That would require maintaining a counter per table rather than a single global counter, which would be bad because then we'd need to store one counter in shared memory for every table, rather than just one, period, which runs up against the fixed sizing of shared memory. I think what Heikki had in mind was that the copy in the index would be the authoritative one, not some image in shared memory. This'd imply dirtying the root page on every insert, as well as increased contention for the root page, so it might have performance problems. Not every insert, just every split. Which might still be a performance problem, but an order of magnitude smaller. I think a bigger issue is where we'd find any space for it. There's no easily-spare space in a GIST page. We could use a larger opaque struct, with the additional field, for the root page, for new indexes. As long as we continue to support the current layout too, that won't break on-disk compatibility. We didn't support unlogged gist indexes before, so we won't have to worry about upgrading unlogged indexes that miss the field. Or if 32 bits is enough for this, we could reuse the right-link. The root page has no right link, so it can be repurposed. This reminds me again that the lack of a metapage in GIST was a serious design error, which we should correct if we ever break on-disk compatibility again. Yeah. I concur that adding such a counter to pg_control is a nonstarter, though. Given that we don't need crash recovery for an unlogged table, could we get away with some variant of NSN that has weaker semantics than XLOG LSNs? One thought I had is that we only generate an NSN when a page is split, and gist never deletes pages, so how about simply using the block number of the newly split page as the NSN? That closes the chance of reinventing page recycling in the future, though. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On 2013-01-15 20:58:00 +0200, Heikki Linnakangas wrote: On 15.01.2013 20:48, Tom Lane wrote: Robert Haasrobertmh...@gmail.com writes: On Tue, Jan 15, 2013 at 1:10 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: Could we stash the counter e.g. in the root page of the index? That would require maintaining a counter per table rather than a single global counter, which would be bad because then we'd need to store one counter in shared memory for every table, rather than just one, period, which runs up against the fixed sizing of shared memory. I think what Heikki had in mind was that the copy in the index would be the authoritative one, not some image in shared memory. This'd imply dirtying the root page on every insert, as well as increased contention for the root page, so it might have performance problems. Not every insert, just every split. Which might still be a performance problem, but an order of magnitude smaller. I might be dense here and I don't really know that code, but if its only splits why not do an XLogInsert(XLOG_GIST_NSN) or something there? Inventing some other form of logging just because its an unlogged table seems like reinventing the wheel. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On Tue, Jan 15, 2013 at 1:58 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: I think what Heikki had in mind was that the copy in the index would be the authoritative one, not some image in shared memory. This'd imply dirtying the root page on every insert, as well as increased contention for the root page, so it might have performance problems. Not every insert, just every split. Which might still be a performance problem, but an order of magnitude smaller. I think that might be acceptable from a performance point of view - after all, if the index is unlogged, you're saving the cost of WAL - but I guess I still prefer a generic solution to this problem (a generalization of GetXLogRecPtrForTemp) rather than a special-purpose solution based on the nitty-gritty of how GiST uses these values. What's the difference between storing this value in pg_control and, say, the OID counter? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
Robert Haas robertmh...@gmail.com writes: I think that might be acceptable from a performance point of view - after all, if the index is unlogged, you're saving the cost of WAL - but I guess I still prefer a generic solution to this problem (a generalization of GetXLogRecPtrForTemp) rather than a special-purpose solution based on the nitty-gritty of how GiST uses these values. What's the difference between storing this value in pg_control and, say, the OID counter? Well, the modularity argument is that GiST shouldn't have any special privileges compared to a third-party index AM. (I realize that third-party AMs already have problems plugging into WAL replay, but that doesn't mean we should create more problems.) We could possibly dodge that objection by regarding the global counter as some sort of generic unlogged operation counter, available to anybody who needs it. It would be good to have a plausible example of something else needing it, but assume somebody can think of one. The bigger issue is that the reason we don't have to update pg_control every other millisecond is that the OID counter is capable of tracking its state between checkpoints without touching pg_control, that is it can emit WAL records to track its increments. I think that we should insist that GiST do likewise, even if we give it some space in pg_control. Remember that pg_control is a single point of failure for the database, and the more often it's written to, the more likely it is that something will go wrong there. So I guess what would make sense to me is that we invent an unlogged ops counter that is managed exactly like the OID counter, including having WAL records that are treated as consuming some number of values in advance. If it's 64 bits wide then the WAL records could safely be made to consume quite a lot of values, like a thousand or so, thus reducing the actual WAL I/O burden to about nothing. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On Tue, Jan 15, 2013 at 4:26 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: I think that might be acceptable from a performance point of view - after all, if the index is unlogged, you're saving the cost of WAL - but I guess I still prefer a generic solution to this problem (a generalization of GetXLogRecPtrForTemp) rather than a special-purpose solution based on the nitty-gritty of how GiST uses these values. What's the difference between storing this value in pg_control and, say, the OID counter? Well, the modularity argument is that GiST shouldn't have any special privileges compared to a third-party index AM. (I realize that third-party AMs already have problems plugging into WAL replay, but that doesn't mean we should create more problems.) We could possibly dodge that objection by regarding the global counter as some sort of generic unlogged operation counter, available to anybody who needs it. It would be good to have a plausible example of something else needing it, but assume somebody can think of one. The bigger issue is that the reason we don't have to update pg_control every other millisecond is that the OID counter is capable of tracking its state between checkpoints without touching pg_control, that is it can emit WAL records to track its increments. I think that we should insist that GiST do likewise, even if we give it some space in pg_control. Remember that pg_control is a single point of failure for the database, and the more often it's written to, the more likely it is that something will go wrong there. So I guess what would make sense to me is that we invent an unlogged ops counter that is managed exactly like the OID counter, including having WAL records that are treated as consuming some number of values in advance. If it's 64 bits wide then the WAL records could safely be made to consume quite a lot of values, like a thousand or so, thus reducing the actual WAL I/O burden to about nothing. I didn't look at the actual patch (silly me?) but the only time you need to update the control file is when writing the shutdown checkpoint just before stopping the database server. If the server crashes, it's OK to roll the value back to some smaller value, because unlogged relations will be reset anyway. And while the server is running the information can live in a shared memory copy protected by a spinlock. So the control file traffic should be limited to once per server lifetime, AFAICS. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
Hi Robert / Tom I think to support GiST with unlogged table we need to do three things: 1. Teach the buffer manager that the LSN of a page not marked BM_PERMANENT can be ignored 2. Teach GetXLogRecPtrForTemp() to allocate fake LSNs for GiST buffers using a 64-bit, counter that is persisted across clean shutdowns 3. Remove the error checks that prevent creating an unlogged GiST index and update the documentation accordingly PFA, patch which try to do above things. For (2), I have added a new function called, GetXLogRecPtrForUnloogedRel() which returns a fake LSN for GiST indexes. However, I have removed GetXLogRecPtrForTemp() function and added its functionality inside this new function itself to avoid complexity. With this patch I am able to create GiST for unlogged tables. It works well with my examples. Also make check has no issues. Please have a look and let me know your views. Thanks On Sat, Dec 18, 2010 at 7:20 AM, Robert Haas robertmh...@gmail.com wrote: On Fri, Dec 17, 2010 at 4:17 PM, Tom Lane t...@sss.pgh.pa.us wrote: Since these bits will only be set/cleared when the buffer mapping is changed, can we examine this bit without taking the spinlock? Only if you're willing for the result to be unreliable. If we read the bits while someone else is writing them, we'll get either the old or the new value, but the BM_FLUSH_XLOG bit will be the same either way. When FlushBuffer() is called, a shared content log and a pin are held, so the buffer can't be renamed under us. If that's really unacceptable, we can do something like the attached, but I think this is unnecessarily gross. We already assume in other places that the read or write of an integer is atomic. In this case we don't even need it to be fully atomic - we just need the particular byte that contains this bit not to go through some intermediate state where the bit is unset. Is there really a memory architecture out there that's crazy enough to let such a thing happen? If so, I'd expect things to be breaking right and left on that machine anyway. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Jeevan B Chalke Senior Software Engineer, RD 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. support_unlogged_gist_indexes.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On Tue, Dec 14, 2010 at 5:14 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Dec 14, 2010 at 4:55 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Dec 14, 2010 at 4:24 PM, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: Hmm, the first idea that comes to mind is to use a counter like the GetXLogRecPtrForTemp() counter I used for temp tables, but global, in shared memory. However, that's a bit problematic because if we store a value from that counter to LSN, it's possible that the counter overtakes the XLOG insert location, and you start to get xlog flush errors. We could avoid that if we added a new field to the GiST page header, and used that to store the value in the parent page instead of the LSN. That doesn't seem ideal, either, because now you're eating up some number of bytes per page in every GIST index just on the off chance that one of them is unlogged. On-disk compatibility seems problematic here as well. Good point. Given the foregoing discussion, I see only two possible paths forward here. 1. Just decide that that unlogged tables can't have GIST indexes, at least until someone figures out a way to make it work. That's sort of an annoying limitation, but I think we could live with it. 2. Write WAL records even though the GIST index is supposedly unlogged. We could either (1) write the usual XLOG_GIST_* records, and arrange to ignore them on replay when the relation in question is unlogged, or (2) write an XLOG_NOOP record to advance the current LSN. The latter sounds safer to me, but it will mean that the XLOG code for GIST needs three separate cases (temp, perm, unlogged). Either way we give up a significant chunk of the benefit of making the relation unlogged in the first place. Thoughts? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
Given the foregoing discussion, I see only two possible paths forward here. 1. Just decide that that unlogged tables can't have GIST indexes, at least until someone figures out a way to make it work. That's sort of an annoying limitation, but I think we could live with it. +1 In the small subset of situations that need unlogged tables, I would think the subset of those that need gist is exceedingly small. Unless someone can come up with a use case that needs both unlogged and gist, I'd vote not to spend time on it. (And, if ever someone does come along with a really good use, then time can be put toward it). -Andy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
Robert Haas robertmh...@gmail.com writes: Given the foregoing discussion, I see only two possible paths forward here. 1. Just decide that that unlogged tables can't have GIST indexes, at least until someone figures out a way to make it work. That's sort of an annoying limitation, but I think we could live with it. 2. Write WAL records even though the GIST index is supposedly unlogged. We could either (1) write the usual XLOG_GIST_* records, and arrange to ignore them on replay when the relation in question is unlogged, or (2) write an XLOG_NOOP record to advance the current LSN. The latter sounds safer to me, but it will mean that the XLOG code for GIST needs three separate cases (temp, perm, unlogged). Either way we give up a significant chunk of the benefit of making the relation unlogged in the first place. Thoughts? IIUC, the problem is that the bufmgr might think that a GIST NSN is an LSN that should affect when to force out a dirty buffer? What if we taught it the difference? We could for example dedicate a pd_flags bit to marking pages whose pd_lsn isn't actually an LSN. This solution would probably imply that all pages in the shared buffer pool have to have a standard PageHeaderData header, not just an LSN at the front as is assumed now. But that doesn't seem like a bad thing to me, unless maybe we were dumb enough to not use a standard page header in some of the secondary forks. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On 17.12.2010 21:07, Tom Lane wrote: IIUC, the problem is that the bufmgr might think that a GIST NSN is an LSN that should affect when to force out a dirty buffer? What if we taught it the difference? We could for example dedicate a pd_flags bit to marking pages whose pd_lsn isn't actually an LSN. This solution would probably imply that all pages in the shared buffer pool have to have a standard PageHeaderData header, not just an LSN at the front as is assumed now. But that doesn't seem like a bad thing to me, unless maybe we were dumb enough to not use a standard page header in some of the secondary forks. I'm not very fond of expanding buffer manager's knowledge of the page layout. How about a new flag in the buffer desc, BM_UNLOGGED? There was some talk about skipping flushing of unlogged tables at checkpoints, I think we'd need BM_UNLOGGED for that anyway. Or I guess we could hang that behavior on the pd_flags bit too, but it doesn't seem like the right place for that information. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes: On 17.12.2010 21:07, Tom Lane wrote: IIUC, the problem is that the bufmgr might think that a GIST NSN is an LSN that should affect when to force out a dirty buffer? What if we taught it the difference? We could for example dedicate a pd_flags bit to marking pages whose pd_lsn isn't actually an LSN. I'm not very fond of expanding buffer manager's knowledge of the page layout. How about a new flag in the buffer desc, BM_UNLOGGED? That could work too, if you can explain how the flag comes to be set without a bunch of ugliness all over the system. I don't want callers of ReadBuffer to have to supply the bit for example. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On Fri, Dec 17, 2010 at 2:07 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: IIUC, the problem is that the bufmgr might think that a GIST NSN is an LSN that should affect when to force out a dirty buffer? What if we taught it the difference? We could for example dedicate a pd_flags bit to marking pages whose pd_lsn isn't actually an LSN. This solution would probably imply that all pages in the shared buffer pool have to have a standard PageHeaderData header, not just an LSN at the front as is assumed now. But that doesn't seem like a bad thing to me, unless maybe we were dumb enough to not use a standard page header in some of the secondary forks. Ah, interesting idea. visibilitymap.c has this: #define MAPSIZE (BLCKSZ - MAXALIGN(SizeOfPageHeaderData)) and fsm_internals.h has this: #define NodesPerPage (BLCKSZ - MAXALIGN(SizeOfPageHeaderData) - \ offsetof(FSMPageData, fp_nodes)) ...which seems to imply, at least on a quick look, that we might be OK, as far as the extra forks go. I have a vague recollection that the documentation says somewhere that a full page header on every page isn't required. And I'm a little worried about my ability to track down every place in the system that might get broken by a change in this area. What do you think about committing the unlogged tables patch initially without support for GIST indexes, and working on fixing this as a separate commit? Another possibly-useful thing about mandating a full page header for every page is that it might give us a way of avoiding unnecessary full page writes. As I wrote previously: However, I think we can avoid this too, by allocating an additional bit in pd_flags, PD_FPI. Instead of emitting an FPI when the old LSN precedes the redo pointer, we'll emit an FPI when the FPI bit is set (in which case we'll also clear the bit) OR when the old LSN precedes the redo pointer. Upon emitting a WAL record that is torn-page safe (such as a freeze or all-visible record), we'll pass a flag to XLogInsert that arranges to suppress FPIs, bump the LSN, and set PD_FPI. That way, if the page is touched again before the next checkpoint by an operation that does NOT suppress FPI, one will be emitted then. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
Robert Haas robertmh...@gmail.com writes: Another possibly-useful thing about mandating a full page header for every page is that it might give us a way of avoiding unnecessary full page writes. As I wrote previously: Could we do that via a bufmgr status bit, instead? Heikki's idea has the merit that it actually reduces bufmgr's knowledge of page headers, rather than increasing it (since a buffer marked UNLOGGED would need no assumptions at all about its content). regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On Fri, Dec 17, 2010 at 2:22 PM, Tom Lane t...@sss.pgh.pa.us wrote: Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes: On 17.12.2010 21:07, Tom Lane wrote: IIUC, the problem is that the bufmgr might think that a GIST NSN is an LSN that should affect when to force out a dirty buffer? What if we taught it the difference? We could for example dedicate a pd_flags bit to marking pages whose pd_lsn isn't actually an LSN. I'm not very fond of expanding buffer manager's knowledge of the page layout. How about a new flag in the buffer desc, BM_UNLOGGED? That could work too, if you can explain how the flag comes to be set without a bunch of ugliness all over the system. I don't want callers of ReadBuffer to have to supply the bit for example. That's easy enough to solve - ReadBuffer() takes a Relation as an argument, so you can easily check RelationNeedsWAL(reln). I guess the question is whether it's right to conflate table is unlogged with LSN is fake. It's not immediately obvious to me that those concepts are isomorphic, although though the reverse isn't obvious to me either. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On Fri, Dec 17, 2010 at 2:31 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: Another possibly-useful thing about mandating a full page header for every page is that it might give us a way of avoiding unnecessary full page writes. As I wrote previously: Could we do that via a bufmgr status bit, instead? Heikki's idea has the merit that it actually reduces bufmgr's knowledge of page headers, rather than increasing it (since a buffer marked UNLOGGED would need no assumptions at all about its content). That was my first thought, but it doesn't work. The buffer could be evicted from shared_buffers and read back in. If a checkpoint intervenes meanwhile, we're OK, but otherwise you fail to emit an otherwise-needed FPI. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On 17.12.2010 21:32, Robert Haas wrote: I guess the question is whether it's right to conflate table is unlogged with LSN is fake. It's not immediately obvious to me that those concepts are isomorphic, although though the reverse isn't obvious to me either. The buffer manager only needs to know if it has to flush the WAL before writing the page to disk. The flag just means that the buffer manager never needs to do that for this buffer. You're still free to store a real LSN there if you want to, it just won't cause any WAL flushes. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes: On 17.12.2010 21:32, Robert Haas wrote: I guess the question is whether it's right to conflate table is unlogged with LSN is fake. It's not immediately obvious to me that those concepts are isomorphic, although though the reverse isn't obvious to me either. The buffer manager only needs to know if it has to flush the WAL before writing the page to disk. The flag just means that the buffer manager never needs to do that for this buffer. You're still free to store a real LSN there if you want to, it just won't cause any WAL flushes. Yeah. I think that BM_UNLOGGED might be a poor choice for the flag name, just because it overstates what the bufmgr needs to assume. It might be better to reverse the flag sense, and have a new flag that *is* set if the page contains an LSN that we have to check against WAL. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On Fri, Dec 17, 2010 at 3:03 PM, Tom Lane t...@sss.pgh.pa.us wrote: Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes: On 17.12.2010 21:32, Robert Haas wrote: I guess the question is whether it's right to conflate table is unlogged with LSN is fake. It's not immediately obvious to me that those concepts are isomorphic, although though the reverse isn't obvious to me either. The buffer manager only needs to know if it has to flush the WAL before writing the page to disk. The flag just means that the buffer manager never needs to do that for this buffer. You're still free to store a real LSN there if you want to, it just won't cause any WAL flushes. Yeah. I think that BM_UNLOGGED might be a poor choice for the flag name, just because it overstates what the bufmgr needs to assume. It might be better to reverse the flag sense, and have a new flag that *is* set if the page contains an LSN that we have to check against WAL. I was actually thinking of adding BM_UNLOGGED even before this discussion, because that would allow unlogged buffers to be excluded from non-shutdown checkpoints. We could add two flags with different semantics that take on, under present rules, the same value, but I'd be disinclined to burn the extra bit without a concrete need. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
Robert Haas robertmh...@gmail.com writes: On Fri, Dec 17, 2010 at 3:03 PM, Tom Lane t...@sss.pgh.pa.us wrote: Yeah. I think that BM_UNLOGGED might be a poor choice for the flag name, just because it overstates what the bufmgr needs to assume. I was actually thinking of adding BM_UNLOGGED even before this discussion, because that would allow unlogged buffers to be excluded from non-shutdown checkpoints. We could add two flags with different semantics that take on, under present rules, the same value, but I'd be disinclined to burn the extra bit without a concrete need. bufmgr is currently using eight bits out of a 16-bit flag field, and IIRC at least five of those have been there since the beginning. So our accretion rate is something like one bit every four years. I think not being willing to use two bits to describe two unrelated behaviors is penny-wise and pound-foolish --- bufmgr is already complicated enough, let's not add useless barriers to readability. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On Fri, Dec 17, 2010 at 3:15 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Fri, Dec 17, 2010 at 3:03 PM, Tom Lane t...@sss.pgh.pa.us wrote: Yeah. I think that BM_UNLOGGED might be a poor choice for the flag name, just because it overstates what the bufmgr needs to assume. I was actually thinking of adding BM_UNLOGGED even before this discussion, because that would allow unlogged buffers to be excluded from non-shutdown checkpoints. We could add two flags with different semantics that take on, under present rules, the same value, but I'd be disinclined to burn the extra bit without a concrete need. bufmgr is currently using eight bits out of a 16-bit flag field, and IIRC at least five of those have been there since the beginning. So our accretion rate is something like one bit every four years. I think not being willing to use two bits to describe two unrelated behaviors is penny-wise and pound-foolish --- bufmgr is already complicated enough, let's not add useless barriers to readability. Allright, what do you want to call the other bit, then? BM_SKIP_XLOG_FLUSH? I have a feeling we may also be creating BM_UNTIDY rather soon, per previous discussion of hint bit I/O. Since these bits will only be set/cleared when the buffer mapping is changed, can we examine this bit without taking the spinlock? If not, we're going to have to stick an extra spinlock acquire/release into FlushBuffer(), which sounds rather unappealing. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
Robert Haas robertmh...@gmail.com writes: Allright, what do you want to call the other bit, then? BM_SKIP_XLOG_FLUSH? Like I said, I'd be tempted to invert the sense, so that the flag is set for normal relations. Then it becomes something like BM_FLUSH_XLOG. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
[ hit send too soon ... ] Robert Haas robertmh...@gmail.com writes: Since these bits will only be set/cleared when the buffer mapping is changed, can we examine this bit without taking the spinlock? Only if you're willing for the result to be unreliable. In the case of the xlog flush bit, I don't believe an extra locking cycle should be necessary anyway, as you surely had the lock when you found the page to be dirty in the first place. You could grab the bit then. I'm not sure where you envision checking the other bit, but I doubt it can be all that far removed from a lock acquisition. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On Fri, Dec 17, 2010 at 4:17 PM, Tom Lane t...@sss.pgh.pa.us wrote: Since these bits will only be set/cleared when the buffer mapping is changed, can we examine this bit without taking the spinlock? Only if you're willing for the result to be unreliable. If we read the bits while someone else is writing them, we'll get either the old or the new value, but the BM_FLUSH_XLOG bit will be the same either way. When FlushBuffer() is called, a shared content log and a pin are held, so the buffer can't be renamed under us. If that's really unacceptable, we can do something like the attached, but I think this is unnecessarily gross. We already assume in other places that the read or write of an integer is atomic. In this case we don't even need it to be fully atomic - we just need the particular byte that contains this bit not to go through some intermediate state where the bit is unset. Is there really a memory architecture out there that's crazy enough to let such a thing happen? If so, I'd expect things to be breaking right and left on that machine anyway. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company bm-flush-xlog-paranoid.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] unlogged tables vs. GIST
On Sat, Nov 13, 2010 at 9:09 PM, Robert Haas robertmh...@gmail.com wrote: The fact that it's easy doesn't make it workable. I would point out for starters that AMs might (do) put WAL locations and/or XIDs into indexes. Occasionally copying very old LSNs or XIDs back into active files seems pretty dangerous. I haven't examined the GIST, GIN, or hash index code in detail so I am not sure whether there are any hazards there; the btree case does not seem to have any issues of this type. Certainly, if an index AM puts an XID into an empty index, that's gonna break. I would consider that a pretty odd thing to do, though. An LSN seems less problematic since the LSN space does not wrap; it should just look like an index that was created a long time ago and never updated (which, in effect, it is). I'm still not convinced there's any hazard of this type, but there is, apparently, a problem with failing to emit XLOG records for GIST indexes, because they apparently use LSNs to detect concurrent page splits (see Heikki's commit on November 16th, aka 2edc5cd493ce3d7834026970e9d3cd00e203f51a) and the hack he inserted to work around that problem for temporary tables isn't going to work for unlogged tables. I suppose we could disallow unlogged GIST indexes. Or we could allow them but still XLOG some operations anyway to make sure that the LSN advances at the appropriate time. That seems pretty ugly, though. Any other ideas? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On 14.12.2010 23:06, Robert Haas wrote: On Sat, Nov 13, 2010 at 9:09 PM, Robert Haasrobertmh...@gmail.com wrote: The fact that it's easy doesn't make it workable. I would point out for starters that AMs might (do) put WAL locations and/or XIDs into indexes. Occasionally copying very old LSNs or XIDs back into active files seems pretty dangerous. I haven't examined the GIST, GIN, or hash index code in detail so I am not sure whether there are any hazards there; the btree case does not seem to have any issues of this type. Certainly, if an index AM puts an XID into an empty index, that's gonna break. I would consider that a pretty odd thing to do, though. An LSN seems less problematic since the LSN space does not wrap; it should just look like an index that was created a long time ago and never updated (which, in effect, it is). I'm still not convinced there's any hazard of this type, but there is, apparently, a problem with failing to emit XLOG records for GIST indexes, because they apparently use LSNs to detect concurrent page splits (see Heikki's commit on November 16th, aka 2edc5cd493ce3d7834026970e9d3cd00e203f51a) and the hack he inserted to work around that problem for temporary tables isn't going to work for unlogged tables. I suppose we could disallow unlogged GIST indexes. Or we could allow them but still XLOG some operations anyway to make sure that the LSN advances at the appropriate time. That seems pretty ugly, though. Any other ideas? Hmm, the first idea that comes to mind is to use a counter like the GetXLogRecPtrForTemp() counter I used for temp tables, but global, in shared memory. However, that's a bit problematic because if we store a value from that counter to LSN, it's possible that the counter overtakes the XLOG insert location, and you start to get xlog flush errors. We could avoid that if we added a new field to the GiST page header, and used that to store the value in the parent page instead of the LSN. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On Tue, Dec 14, 2010 at 4:24 PM, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: Hmm, the first idea that comes to mind is to use a counter like the GetXLogRecPtrForTemp() counter I used for temp tables, but global, in shared memory. However, that's a bit problematic because if we store a value from that counter to LSN, it's possible that the counter overtakes the XLOG insert location, and you start to get xlog flush errors. We could avoid that if we added a new field to the GiST page header, and used that to store the value in the parent page instead of the LSN. That doesn't seem ideal, either, because now you're eating up some number of bytes per page in every GIST index just on the off chance that one of them is unlogged. Unless there's a way to do it only for unlogged GIST indexes, but it seems like that could be messy. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
Robert Haas robertmh...@gmail.com writes: On Tue, Dec 14, 2010 at 4:24 PM, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: Hmm, the first idea that comes to mind is to use a counter like the GetXLogRecPtrForTemp() counter I used for temp tables, but global, in shared memory. However, that's a bit problematic because if we store a value from that counter to LSN, it's possible that the counter overtakes the XLOG insert location, and you start to get xlog flush errors. We could avoid that if we added a new field to the GiST page header, and used that to store the value in the parent page instead of the LSN. That doesn't seem ideal, either, because now you're eating up some number of bytes per page in every GIST index just on the off chance that one of them is unlogged. On-disk compatibility seems problematic here as well. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unlogged tables vs. GIST
On Tue, Dec 14, 2010 at 4:55 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Dec 14, 2010 at 4:24 PM, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: Hmm, the first idea that comes to mind is to use a counter like the GetXLogRecPtrForTemp() counter I used for temp tables, but global, in shared memory. However, that's a bit problematic because if we store a value from that counter to LSN, it's possible that the counter overtakes the XLOG insert location, and you start to get xlog flush errors. We could avoid that if we added a new field to the GiST page header, and used that to store the value in the parent page instead of the LSN. That doesn't seem ideal, either, because now you're eating up some number of bytes per page in every GIST index just on the off chance that one of them is unlogged. On-disk compatibility seems problematic here as well. Good point. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
