Thread: unlogged tables vs. GIST
On Sat, Nov 13, 2010 at 9:09 PM, Robert Haas <robertmhaas@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
On 14.12.2010 23:06, Robert Haas wrote: > On Sat, Nov 13, 2010 at 9:09 PM, Robert Haas<robertmhaas@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
On Tue, Dec 14, 2010 at 4:24 PM, Heikki Linnakangas <heikki.linnakangas@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
Robert Haas <robertmhaas@gmail.com> writes: > On Tue, Dec 14, 2010 at 4:24 PM, Heikki Linnakangas > <heikki.linnakangas@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
On Tue, Dec 14, 2010 at 4:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Tue, Dec 14, 2010 at 4:24 PM, Heikki Linnakangas >> <heikki.linnakangas@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
On Tue, Dec 14, 2010 at 5:14 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Dec 14, 2010 at 4:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Robert Haas <robertmhaas@gmail.com> writes: >>> On Tue, Dec 14, 2010 at 4:24 PM, Heikki Linnakangas >>> <heikki.linnakangas@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 thatthe 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
> > 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
Robert Haas <robertmhaas@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
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
Heikki Linnakangas <heikki.linnakangas@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
On Fri, Dec 17, 2010 at 2:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@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
Robert Haas <robertmhaas@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
On Fri, Dec 17, 2010 at 2:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Heikki Linnakangas <heikki.linnakangas@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
On Fri, Dec 17, 2010 at 2:31 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@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
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
Heikki Linnakangas <heikki.linnakangas@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
On Fri, Dec 17, 2010 at 3:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Heikki Linnakangas <heikki.linnakangas@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
Robert Haas <robertmhaas@gmail.com> writes: > On Fri, Dec 17, 2010 at 3:03 PM, Tom Lane <tgl@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
On Fri, Dec 17, 2010 at 3:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Fri, Dec 17, 2010 at 3:03 PM, Tom Lane <tgl@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
Robert Haas <robertmhaas@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
[ hit send too soon ... ] Robert Haas <robertmhaas@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
On Fri, Dec 17, 2010 at 4:17 PM, Tom Lane <tgl@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
Attachment
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
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 <robertmhaas@gmail.com> wrote:
On Fri, Dec 17, 2010 at 4:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:If we read the bits while someone else is writing them, we'll get
>> 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.
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.
--
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, 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.
Attachment
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
On Tue, Jan 15, 2013 at 1:10 PM, Heikki Linnakangas <hlinnakangas@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
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
Robert Haas <robertmhaas@gmail.com> writes: > On Tue, Jan 15, 2013 at 1:10 PM, Heikki Linnakangas > <hlinnakangas@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
On Tue, Jan 15, 2013 at 1:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Tue, Jan 15, 2013 at 1:10 PM, Heikki Linnakangas >> <hlinnakangas@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
On 15.01.2013 20:48, Tom Lane wrote: > Robert Haas<robertmhaas@gmail.com> writes: >> On Tue, Jan 15, 2013 at 1:10 PM, Heikki Linnakangas >> <hlinnakangas@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
On 2013-01-15 20:58:00 +0200, Heikki Linnakangas wrote: > On 15.01.2013 20:48, Tom Lane wrote: > >Robert Haas<robertmhaas@gmail.com> writes: > >>On Tue, Jan 15, 2013 at 1:10 PM, Heikki Linnakangas > >><hlinnakangas@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
On Tue, Jan 15, 2013 at 1:58 PM, Heikki Linnakangas <hlinnakangas@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
Robert Haas <robertmhaas@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
On Tue, Jan 15, 2013 at 4:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@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
On Wed, Jan 16, 2013 at 3:24 AM, Robert Haas <robertmhaas@gmail.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.
Thanks
I didn't look at the actual patch (silly me?) but the only time youOn Tue, Jan 15, 2013 at 4:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@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.
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.
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
--
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.
Attachment
On Wed, Jan 23, 2013 at 4:04 AM, Jeevan Chalke <jeevan.chalke@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
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
On Mon, Jan 28, 2013 at 4:04 AM, Heikki Linnakangas <hlinnakangas@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
Hi 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.
On Mon, Jan 28, 2013 at 2:34 PM, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:
On 23.01.2013 17:30, Robert Haas 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.
Attachment
Hi,
Any review comments on this ?On Tue, Jan 29, 2013 at 6:03 PM, Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote:
Hi Heikki,On Mon, Jan 28, 2013 at 2:34 PM, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:On 23.01.2013 17:30, Robert Haas 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.cI 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.cI 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.
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
Attachment
Hi 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.
On Mon, Feb 11, 2013 at 7:28 PM, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:
On 11.02.2013 08:44, Jeevan Chalke wrote:Sorry for the delay.Hi,
Any review comments on this ?
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, 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.