Re: snapshot too old, configured by time

On Thu, Mar 24, 2016 at 2:24 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

> I have been looking at 4a, the test module, and things are looking
> good IMO. Something that I think would be adapted would be to define
> the options for isolation tests in a variable, like ISOLATION_OPTS to
> allow MSVC scripts to fetch those option values more easily.

Maybe, but that seems like material for a separate patch.

> +submake-test_decoding:
> +   $(MAKE) -C $(top_builddir)/src/test/modules/snapshot_too_old
> The target name here is incorrect. This should refer to snapshot_too_old.

Good catch.  Fixed.

So far, pending the resolution of the suggestion to add three new
parameters to BufferGetPage in 450 places that otherwise don't need
to be touched, this is the only change from the flurry of recent
testing and review, so I'm holding off on posting a new patch just
for this.

> Regarding the main patch:
> +        <primary><varname>old_snapshot_threshold</> configuration
> parameter</primary>
> snapshot_valid_limit?

There have already been responses supporting
old_snapshot_threshold, so I would need to hear a few more votes to
consider a change.

>     page = BufferGetPage(buf);
> +   TestForOldSnapshot(scan->xs_snapshot, rel, page);
> This is a sequence repeated many times in this patch, a new routine,
> say BufferGetPageExtended with a uint8 flag, one flag being used to
> test old snapshots would be more adapted. But this would require
> creating a header dependency between the buffer manager and
> SnapshotData.. Or more simply we may want a common code path when
> fetching a page that a backend is going to use to fetch tuples. I am
> afraid of TestForOldSnapshot() being something that could be easily
> forgotten in code paths introduced in future patches...

Let's keep that discussion on the other branch of this thread.

> +   if (whenTaken < 0)
> +   {
> +       elog(DEBUG1,
> +            "MaintainOldSnapshotTimeMapping called with negative
> whenTaken = %ld",
> +            (long) whenTaken);
> +       return;
> +   }
> +   if (!TransactionIdIsNormal(xmin))
> +   {
> +       elog(DEBUG1,
> +            "MaintainOldSnapshotTimeMapping called with xmin = %lu",
> +            (unsigned long) xmin);
> +       return;
> +   }
> Material for two assertions?

You omitted the immediately preceding comment block:

+    /*
+     * We don't want to do something stupid with unusual values, but we don't
+     * want to litter the log with warnings or break otherwise normal
+     * processing for this feature; so if something seems unreasonable, just
+     * log at DEBUG level and return without doing anything.
+     */

I'm not clear that more drastic action is a good idea, since the
"fallback" is existing behavior.  I fear that doing something more
aggressive might force other logic to become more precise about
aggressive clean-up, adding overhead beyond the value gained.

Thanks for the review!

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