Re: fixing old_snapshot_threshold's time->xid mapping

On Mon, Apr 20, 2020 at 11:31 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Mon, Apr 20, 2020 at 12:10 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > I have started reviewing these patches. I think, the fixes looks right to me.
> >
> > + LWLockAcquire(OldSnapshotTimeMapLock, LW_SHARED);
> > + mapping->head_offset = oldSnapshotControl->head_offset;
> > + mapping->head_timestamp = oldSnapshotControl->head_timestamp;
> > + mapping->count_used = oldSnapshotControl->count_used;
> > + for (int i = 0; i < OLD_SNAPSHOT_TIME_MAP_ENTRIES; ++i)
> > + mapping->xid_by_minute[i] = oldSnapshotControl->xid_by_minute[i];
> > + LWLockRelease(OldSnapshotTimeMapLock);
> >
> > I think memcpy would be a better choice instead of looping it for all
> > the entries, since we are doing this under a lock?
>
> When I did it that way, it complained about "const" and I couldn't
> immediately figure out how to fix it.

I think we can typecast to (const void *). After below change, I did
not get the warning.

diff --git a/contrib/old_snapshot/time_mapping.c
b/contrib/old_snapshot/time_mapping.c
index 37e0055..cc53bdd 100644
--- a/contrib/old_snapshot/time_mapping.c
+++ b/contrib/old_snapshot/time_mapping.c
@@ -94,8 +94,9 @@ GetOldSnapshotTimeMapping(void)
mapping->head_offset = oldSnapshotControl->head_offset;
mapping->head_timestamp = oldSnapshotControl->head_timestamp;
mapping->count_used = oldSnapshotControl->count_used;
- for (int i = 0; i < OLD_SNAPSHOT_TIME_MAP_ENTRIES; ++i)
- mapping->xid_by_minute[i] =
oldSnapshotControl->xid_by_minute[i];
+ memcpy(mapping->xid_by_minute,
+ (const void *) oldSnapshotControl->xid_by_minute,
+ sizeof(TransactionId) * OLD_SNAPSHOT_TIME_MAP_ENTRIES);
LWLockRelease(OldSnapshotTimeMapLock);

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