Re: fixing old_snapshot_threshold's time->xid mapping

On Tue, Apr 21, 2020 at 4:52 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Tue, Apr 21, 2020 at 3:44 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> >
> > On Tue, Apr 21, 2020 at 2:05 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> > > As before, these two apply on top of Robert's patches (or at least his
> > > 0001 and 0002).
> >
> > While trying to figure out if Robert's 0003 patch was correct, I added
> > yet another patch to this stack to test it. 0006 does basic xid map
> > maintenance that exercises the cases 0003 fixes, and I think it
> > demonstrates that they now work correctly.
>
> +1, I think we should also add a way to test the case, where we
> advance the timestamp by multiple slots. I see that you have such
> case
> e.g
> +# test adding minutes while the map is not full
> +set_time('3000-01-01 02:01:00Z');
> +is(summarize_mapping(), "2|02:00:00|02:01:00");
> +set_time('3000-01-01 02:05:00Z');
> +is(summarize_mapping(), "6|02:00:00|02:05:00");
> +set_time('3000-01-01 02:19:00Z');
> +is(summarize_mapping(), "20|02:00:00|02:19:00");
>
> But, I think we should try to extend it to test that we have put the
> new xid only in those slots where we suppose to and not in other
> slots?.

I feel that we should. probably fix this check as well? Because if ts
> update_ts then it will go to else part then there it will finally
end up in the last slot only so I think we can use this case also as
fast exit.

diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 93a0c04..644d9b1 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -1831,7 +1831,7 @@
TransactionIdLimitedForOldSnapshots(TransactionId recentXmin,

if (!same_ts_as_threshold)
{
- if (ts == update_ts)
+ if (ts >= update_ts)
{
xlimit = latest_xmin;
if (NormalTransactionIdFollows(xlimit,
recentXmin))

This patch can be applied on top of other v5 patches.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com