Re: [HACKERS] Re: [COMMITTERS] pgsql: Prevent "snapshot too old" from trying to return pruned TOAST tu

[Robert Haas]

On Fri, Aug 5, 2016 at 11:05 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, Aug 4, 2016 at 10:58 PM, Robert Haas  wrote:
>>> What is the reason?  We refuse to separate frontend and backend
>>> headers in any sort of principled way?
>
>> That was poorly phrased.  I'll try again: I can't see any reason for
>> using a macro here except that it allows frontend code to compile this
>> without breaking.
>
> Well, the alternative would be to put "#ifndef FRONTEND" around the
> static-inline function.  That's not very pretty either, and it's
> inconsistent with the existing precedent (ie, InitDirtySnapshot).
> Also, it presumes that a non-backend includer actually has defined
> FRONTEND; that seems to be the case for pg_xlogdump but I do not
> think we do that everywhere.

That may not be pretty, but it'd be a lot more transparent.  If I see
#ifndef FRONTEND, I think, oh, that's protecting some stuff that
shouldn't be included in front-end compiles.  If I see a macro, I not
necessarily think of the fact that this may be a way of preventing
that code from being compiled in front-end compiles.

>> Here's a patch.  Is this what you want?
>
> OK by me.

OK, committed.

-- 
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] Re: [COMMITTERS] pgsql: Prevent "snapshot too old" from trying to return pruned TOAST tu

[Tom Lane]

Robert Haas  writes:
> On Thu, Aug 4, 2016 at 10:58 PM, Robert Haas  wrote:
>> What is the reason?  We refuse to separate frontend and backend
>> headers in any sort of principled way?

> That was poorly phrased.  I'll try again: I can't see any reason for
> using a macro here except that it allows frontend code to compile this
> without breaking.

Well, the alternative would be to put "#ifndef FRONTEND" around the
static-inline function.  That's not very pretty either, and it's
inconsistent with the existing precedent (ie, InitDirtySnapshot).
Also, it presumes that a non-backend includer actually has defined
FRONTEND; that seems to be the case for pg_xlogdump but I do not
think we do that everywhere.

> Here's a patch.  Is this what you want?

OK by me.

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

[HACKERS] Re: [COMMITTERS] pgsql: Prevent "snapshot too old" from trying to return pruned TOAST tu

[Robert Haas]

On Thu, Aug 4, 2016 at 10:58 PM, Robert Haas  wrote:
> On Thu, Aug 4, 2016 at 7:23 PM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> Prevent "snapshot too old" from trying to return pruned TOAST tuples.
>>
>> Looks like this patch broke the build on castoroides.  Recommend
>> changing InitToastSnapshot into a macro.  (There's a reason why
>> InitDirtySnapshot is a macro.)
>
> What is the reason?  We refuse to separate frontend and backend
> headers in any sort of principled way?

That was poorly phrased.  I'll try again: I can't see any reason for
using a macro here except that it allows frontend code to compile this
without breaking.  But that doesn't seem like a very good way of
solving that problem.  There's surely no way for a casual reader of
the code to realize that macros can be used here and inline functions
cannot, especially because this works apparently works fine on most
machines, including mine.

Here's a patch.  Is this what you want?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/access/heap/tuptoaster.c b/src/backend/access/heap/tuptoaster.c
index 55b6b41..31b0132 100644
--- a/src/backend/access/heap/tuptoaster.c
+++ b/src/backend/access/heap/tuptoaster.c
@@ -2316,5 +2316,5 @@ init_toast_snapshot(Snapshot toast_snapshot)
 	if (snapshot == NULL)
 		elog(ERROR, "no known snapshots");
 
-	InitToastSnapshot(toast_snapshot, snapshot->lsn, snapshot->whenTaken);
+	InitToastSnapshot(*toast_snapshot, snapshot->lsn, snapshot->whenTaken);
 }
diff --git a/src/include/utils/tqual.h b/src/include/utils/tqual.h
index 8041e7b..fc7328c 100644
--- a/src/include/utils/tqual.h
+++ b/src/include/utils/tqual.h
@@ -104,12 +104,9 @@ extern bool ResolveCminCmaxDuringDecoding(struct HTAB *tuplecid_data,
  * Similarly, some initialization is required for SnapshotToast.  We need
  * to set lsn and whenTaken correctly to support snapshot_too_old.
  */
-static inline void
-InitToastSnapshot(Snapshot snapshot, XLogRecPtr lsn, int64 whenTaken)
-{
-	snapshot->satisfies = HeapTupleSatisfiesToast;
-	snapshot->lsn = lsn;
-	snapshot->whenTaken = whenTaken;
-}
+#define InitToastSnapshot(snapshotdata, l, w)  \
+	((snapshotdata).satisfies = HeapTupleSatisfiesDirty, \
+	 (snapshotdata).lsn = (l),	\
+	 (snapshotdata).whenTaken = (w))
 
 #endif   /* TQUAL_H */

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers