Re: [HACKERS] old_snapshot_threshold's interaction with hash index

[Kevin Grittner]

On Fri, May 6, 2016 at 12:45 AM, Amit Kapila 
wrote:
> On Wed, May 4, 2016 at 7:48 PM, Kevin Grittner  wrote:
>> On Tue, May 3, 2016 at 11:48 AM, Robert Haas 
wrote:
>>
>>> OK, I see now: the basic idea here is that we can't prune based on the
>>> newer XID unless the page LSN is guaranteed to advance whenever data
>>> is removed.  Currently, we attempt to limit bloat in non-unlogged,
>>> non-catalog tables.  You're saying we can instead attempt to limit
>>> bloat only in non-unlogged, non-catalog tables without hash indexes,
>>> and that will fix this issue.  Am I right?
>>
>> As a first cut, something like the attached.
>
> Patch looks good to me.  I have done some testing with hash and
> btree indexes and it works as expected.

Pushed with the addition of a paragraph to the docs regarding this
and some other situations where people have been unclear about what
to expect.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] old_snapshot_threshold's interaction with hash index

[Amit Kapila]

On Wed, May 4, 2016 at 7:48 PM, Kevin Grittner  wrote:
>
> On Tue, May 3, 2016 at 11:48 AM, Robert Haas 
wrote:
>
> > OK, I see now: the basic idea here is that we can't prune based on the
> > newer XID unless the page LSN is guaranteed to advance whenever data
> > is removed.  Currently, we attempt to limit bloat in non-unlogged,
> > non-catalog tables.  You're saying we can instead attempt to limit
> > bloat only in non-unlogged, non-catalog tables without hash indexes,
> > and that will fix this issue.  Am I right?
>
> As a first cut, something like the attached.
>

Patch looks good to me.  I have done some testing with hash and btree
indexes and it works as expected.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] old_snapshot_threshold's interaction with hash index

[Kevin Grittner]

On Tue, May 3, 2016 at 11:48 AM, Robert Haas  wrote:

> OK, I see now: the basic idea here is that we can't prune based on the
> newer XID unless the page LSN is guaranteed to advance whenever data
> is removed.  Currently, we attempt to limit bloat in non-unlogged,
> non-catalog tables.  You're saying we can instead attempt to limit
> bloat only in non-unlogged, non-catalog tables without hash indexes,
> and that will fix this issue.  Am I right?

As a first cut, something like the attached.

-- 
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 4fecece..49a6c81 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -279,7 +279,6 @@ hashgettuple(IndexScanDesc scan, ScanDirection dir)
 		buf = so->hashso_curbuf;
 		Assert(BufferIsValid(buf));
 		page = BufferGetPage(buf);
-		TestForOldSnapshot(scan->xs_snapshot, rel, page);
 		maxoffnum = PageGetMaxOffsetNumber(page);
 		for (offnum = ItemPointerGetOffsetNumber(current);
 			 offnum <= maxoffnum;
diff --git a/src/backend/access/hash/hashsearch.c b/src/backend/access/hash/hashsearch.c
index eb8c9cd..4825558 100644
--- a/src/backend/access/hash/hashsearch.c
+++ b/src/backend/access/hash/hashsearch.c
@@ -189,7 +189,6 @@ _hash_first(IndexScanDesc scan, ScanDirection dir)
 	/* Read the metapage */
 	metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE);
 	page = BufferGetPage(metabuf);
-	TestForOldSnapshot(scan->xs_snapshot, rel, page);
 	metap = HashPageGetMeta(page);
 
 	/*
@@ -243,7 +242,6 @@ _hash_first(IndexScanDesc scan, ScanDirection dir)
 	/* Fetch the primary bucket page for the bucket */
 	buf = _hash_getbuf(rel, blkno, HASH_READ, LH_BUCKET_PAGE);
 	page = BufferGetPage(buf);
-	TestForOldSnapshot(scan->xs_snapshot, rel, page);
 	opaque = (HashPageOpaque) PageGetSpecialPointer(page);
 	Assert(opaque->hasho_bucket == bucket);
 
@@ -350,7 +348,6 @@ _hash_step(IndexScanDesc scan, Buffer *bufP, ScanDirection dir)
 	_hash_readnext(rel, , , );
 	if (BufferIsValid(buf))
 	{
-		TestForOldSnapshot(scan->xs_snapshot, rel, page);
 		maxoff = PageGetMaxOffsetNumber(page);
 		offnum = _hash_binsearch(page, so->hashso_sk_hash);
 	}
@@ -392,7 +389,6 @@ _hash_step(IndexScanDesc scan, Buffer *bufP, ScanDirection dir)
 	_hash_readprev(rel, , , );
 	if (BufferIsValid(buf))
 	{
-		TestForOldSnapshot(scan->xs_snapshot, rel, page);
 		maxoff = PageGetMaxOffsetNumber(page);
 		offnum = _hash_binsearch_last(page, so->hashso_sk_hash);
 	}
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 432feef..79cc3df 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -5313,6 +5313,52 @@ RelationIdIsInInitFile(Oid relationId)
 }
 
 /*
+ * Tells whether any index for the relation is unlogged.
+ *
+ * Any index using the hash AM is implicitly unlogged.
+ *
+ * Note: There doesn't seem to be any way to have an unlogged index attached
+ * to a permanent table except to create a hash index, but it seems best to
+ * keep this general so that it returns sensible results even when they seem
+ * obvious (like for an unlogged table) and to handle possible future unlogged
+ * indexes on permanent tables.
+ */
+bool
+RelationHasUnloggedIndex(Relation rel)
+{
+	List		   *indexoidlist;
+	ListCell	   *indexoidscan;
+	bool			result = false;
+
+	indexoidlist = RelationGetIndexList(rel);
+
+	foreach(indexoidscan, indexoidlist)
+	{
+		Oid			indexoid = lfirst_oid(indexoidscan);
+		HeapTuple	tp;
+		Form_pg_class reltup;
+
+		tp = SearchSysCache1(RELOID, ObjectIdGetDatum(indexoid));
+		if (!HeapTupleIsValid(tp))
+			elog(ERROR, "cache lookup failed for relation %u", indexoid);
+		reltup = (Form_pg_class) GETSTRUCT(tp);
+
+		if (reltup->relpersistence == RELPERSISTENCE_UNLOGGED
+			|| reltup->relam == HASH_AM_OID)
+			result = true;
+
+		ReleaseSysCache(tp);
+
+		if (result == true)
+			break;
+	}
+
+	list_free(indexoidlist);
+
+	return result;
+}
+
+/*
  * Invalidate (remove) the init file during commit of a transaction that
  * changed one or more of the relation cache entries that are kept in the
  * local init file.
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 0a9a231..e1551a3 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -1590,7 +1590,8 @@ TransactionIdLimitedForOldSnapshots(TransactionId recentXmin,
 		&& old_snapshot_threshold >= 0
 		&& RelationNeedsWAL(relation)
 		&& !IsCatalogRelation(relation)
-		&& !RelationIsAccessibleInLogicalDecoding(relation))
+		&& !RelationIsAccessibleInLogicalDecoding(relation)
+		&& !RelationHasUnloggedIndex(relation))
 	{
 		int64		ts = GetSnapshotCurrentTimestamp();
 		TransactionId xlimit = recentXmin;
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 

Re: [HACKERS] old_snapshot_threshold's interaction with hash index

[Amit Kapila]

On Tue, May 3, 2016 at 9:47 PM, Kevin Grittner  wrote:
>
> On Tue, May 3, 2016 at 11:09 AM, Robert Haas 
wrote:
> > On Tue, May 3, 2016 at 11:46 AM, Kevin Grittner 
wrote:
> >>> Uh, I have no idea how this would be fixed if the PageLSN is zero.  Do
> >>> you?
> >>
> >> Yes, I see three ways, the most obvious of which is what Amit
> >> suggested -- don't do early vacuum on a table which has a hash index.
> >
> > What do you mean by "early VACUUM"?
>
> Both vacuuming and hot-pruning adjust xmin based on calling
> TransactionIdLimitedForOldSnapshots(TransactionId recentXmin,
> Relation relation).  I'm talking about having that function, if all
> other conditions for the override pass, checking for a hash index,
> too.
>
> > Amit suggested disabling
> > HOT-pruning, but HOT-pruning happens completely outside of VACUUM.  It
> > also happens inside VACUUM, so if we disabled HOT pruning, how could
> > we VACUUM at all?  Sorry, I am confused.
>
> I guess we were both talking a bit loosely since (as I mentioned
> above) the function that adjusts the xmin is called for a vacuum or
> pruning.  He mentioned one and I mentioned the other, but it's all
> controlled by TransactionIdLimitedForOldSnapshots().
>

Yes, I think we are saying the same thing here.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] old_snapshot_threshold's interaction with hash index

[Kevin Grittner]

On Tue, May 3, 2016 at 11:48 AM, Robert Haas  wrote:

> OK, I see now: the basic idea here is that we can't prune based on the
> newer XID unless the page LSN is guaranteed to advance whenever data
> is removed.  Currently, we attempt to limit bloat in non-unlogged,
> non-catalog tables.  You're saying we can instead attempt to limit
> bloat only in non-unlogged, non-catalog tables without hash indexes,
> and that will fix this issue.  Am I right?

Right.

I was wondering whether there might be other avenues to the same
end, but that is the most obvious fix.  I'm hesitant to raise the
alternatives because people seem to have entered "panic mode", at
which point alternatives always look scary.

--
Kevin Grittner
EDB: 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] old_snapshot_threshold's interaction with hash index

[Robert Haas]

On Tue, May 3, 2016 at 12:17 PM, Kevin Grittner  wrote:
> On Tue, May 3, 2016 at 11:09 AM, Robert Haas  wrote:
>> On Tue, May 3, 2016 at 11:46 AM, Kevin Grittner  wrote:
 Uh, I have no idea how this would be fixed if the PageLSN is zero.  Do
 you?
>>>
>>> Yes, I see three ways, the most obvious of which is what Amit
>>> suggested -- don't do early vacuum on a table which has a hash index.
>>
>> What do you mean by "early VACUUM"?
>
> Both vacuuming and hot-pruning adjust xmin based on calling
> TransactionIdLimitedForOldSnapshots(TransactionId recentXmin,
> Relation relation).  I'm talking about having that function, if all
> other conditions for the override pass, checking for a hash index,
> too.
>
>> Amit suggested disabling
>> HOT-pruning, but HOT-pruning happens completely outside of VACUUM.  It
>> also happens inside VACUUM, so if we disabled HOT pruning, how could
>> we VACUUM at all?  Sorry, I am confused.
>
> I guess we were both talking a bit loosely since (as I mentioned
> above) the function that adjusts the xmin is called for a vacuum or
> pruning.  He mentioned one and I mentioned the other, but it's all
> controlled by TransactionIdLimitedForOldSnapshots().

OK, I see now: the basic idea here is that we can't prune based on the
newer XID unless the page LSN is guaranteed to advance whenever data
is removed.  Currently, we attempt to limit bloat in non-unlogged,
non-catalog tables.  You're saying we can instead attempt to limit
bloat only in non-unlogged, non-catalog tables without hash indexes,
and that will fix this issue.  Am I right?

-- 
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] old_snapshot_threshold's interaction with hash index

[Kevin Grittner]

On Tue, May 3, 2016 at 11:09 AM, Robert Haas  wrote:
> On Tue, May 3, 2016 at 11:46 AM, Kevin Grittner  wrote:
>>> Uh, I have no idea how this would be fixed if the PageLSN is zero.  Do
>>> you?
>>
>> Yes, I see three ways, the most obvious of which is what Amit
>> suggested -- don't do early vacuum on a table which has a hash index.
>
> What do you mean by "early VACUUM"?

Both vacuuming and hot-pruning adjust xmin based on calling
TransactionIdLimitedForOldSnapshots(TransactionId recentXmin,
Relation relation).  I'm talking about having that function, if all
other conditions for the override pass, checking for a hash index,
too.

> Amit suggested disabling
> HOT-pruning, but HOT-pruning happens completely outside of VACUUM.  It
> also happens inside VACUUM, so if we disabled HOT pruning, how could
> we VACUUM at all?  Sorry, I am confused.

I guess we were both talking a bit loosely since (as I mentioned
above) the function that adjusts the xmin is called for a vacuum or
pruning.  He mentioned one and I mentioned the other, but it's all
controlled by TransactionIdLimitedForOldSnapshots().

> Doesn't this issue also affected indexes on any unlogged table?

That's been covered all along.

--
Kevin Grittner
EDB: 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] old_snapshot_threshold's interaction with hash index

[Robert Haas]

On Tue, May 3, 2016 at 11:46 AM, Kevin Grittner  wrote:
>> Uh, I have no idea how this would be fixed if the PageLSN is zero.  Do
>> you?
>
> Yes, I see three ways, the most obvious of which is what Amit
> suggested -- don't do early vacuum on a table which has a hash index.

What do you mean by "early VACUUM"?  Amit suggested disabling
HOT-pruning, but HOT-pruning happens completely outside of VACUUM.  It
also happens inside VACUUM, so if we disabled HOT pruning, how could
we VACUUM at all?  Sorry, I am confused.

Doesn't this issue also affected indexes on any unlogged table?

-- 
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] old_snapshot_threshold's interaction with hash index

[Kevin Grittner]

On Tue, May 3, 2016 at 10:45 AM, Bruce Momjian  wrote:
> On Mon, May  2, 2016 at 04:02:35PM -0500, Kevin Grittner wrote:
>> On Sun, May 1, 2016 at 1:43 AM, Amit Kapila  wrote:
>> > On Sun, May 1, 2016 at 12:05 PM, Amit Kapila  
>> > wrote:
>> >>
>> >> Currently we do the test for old snapshot (TestForOldSnapshot) for hash
>> >> indexes while scanning them.  Does this test makes any sense for hash
>> >> indexes considering LSN on hash index will always be zero (as hash indexes
>> >> are not WAL-logged)?  It seems to me that PageLSN check in
>> >> TestForOldSnapshot() will always return false which means that the error
>> >> "snapshot too old" won't be generated for hash indexes.
>> >>
>> >> Am I missing something here, if not, then I think we need a way to
>> >> prohibit pruning for hash indexes based on old_snapshot_threshold?
>> >
>> > What I mean to say here is prohibit pruning the relation which has hash
>> > index based on old_snapshot_threshold.
>>
>> Good spot; added to the open issues page.
>
> Uh, I have no idea how this would be fixed if the PageLSN is zero.  Do
> you?

Yes, I see three ways, the most obvious of which is what Amit
suggested -- don't do early vacuum on a table which has a hash index.

-- 
Kevin Grittner
EDB: 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] old_snapshot_threshold's interaction with hash index

[Bruce Momjian]

On Mon, May  2, 2016 at 04:02:35PM -0500, Kevin Grittner wrote:
> On Sun, May 1, 2016 at 1:43 AM, Amit Kapila  wrote:
> > On Sun, May 1, 2016 at 12:05 PM, Amit Kapila  
> > wrote:
> >>
> >> Currently we do the test for old snapshot (TestForOldSnapshot) for hash
> >> indexes while scanning them.  Does this test makes any sense for hash
> >> indexes considering LSN on hash index will always be zero (as hash indexes
> >> are not WAL-logged)?  It seems to me that PageLSN check in
> >> TestForOldSnapshot() will always return false which means that the error
> >> "snapshot too old" won't be generated for hash indexes.
> >>
> >> Am I missing something here, if not, then I think we need a way to
> >> prohibit pruning for hash indexes based on old_snapshot_threshold?
> >
> > What I mean to say here is prohibit pruning the relation which has hash
> > index based on old_snapshot_threshold.
> 
> Good spot; added to the open issues page.

Uh, I have no idea how this would be fixed if the PageLSN is zero.  Do
you?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +


-- 
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] old_snapshot_threshold's interaction with hash index

[Kevin Grittner]

On Sun, May 1, 2016 at 1:43 AM, Amit Kapila  wrote:
> On Sun, May 1, 2016 at 12:05 PM, Amit Kapila  wrote:
>>
>> Currently we do the test for old snapshot (TestForOldSnapshot) for hash
>> indexes while scanning them.  Does this test makes any sense for hash
>> indexes considering LSN on hash index will always be zero (as hash indexes
>> are not WAL-logged)?  It seems to me that PageLSN check in
>> TestForOldSnapshot() will always return false which means that the error
>> "snapshot too old" won't be generated for hash indexes.
>>
>> Am I missing something here, if not, then I think we need a way to
>> prohibit pruning for hash indexes based on old_snapshot_threshold?
>
> What I mean to say here is prohibit pruning the relation which has hash
> index based on old_snapshot_threshold.

Good spot; added to the open issues page.

Thanks!

-- 
Kevin Grittner
EDB: 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] old_snapshot_threshold's interaction with hash index

[Amit Kapila]

On Sun, May 1, 2016 at 12:05 PM, Amit Kapila 
wrote:

> Currently we do the test for old snapshot (TestForOldSnapshot) for hash
> indexes while scanning them.  Does this test makes any sense for hash
> indexes considering LSN on hash index will always be zero (as hash indexes
> are not WAL-logged)?  It seems to me that PageLSN check in
> TestForOldSnapshot() will always return false which means that the error
> "snapshot too old" won't be generated for hash indexes.
>
> Am I missing something here, if not, then I think we need a way to
> prohibit pruning for hash indexes based on old_snapshot_threshold?
>
>
What I mean to say here is prohibit pruning the relation which has hash
index based on old_snapshot_threshold.



With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com