On Tue, Feb 6, 2024 at 2:16 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > Thoughts? > > > +1 for the idea, here are some comments on 0002, I will review other > patches soon and respond.
Thanks for looking at it. > + <structfield>inactive_at</structfield> <type>timestamptz</type> > > Maybe we can change the field name to 'last_inactive_at'? or maybe the > comment can explain timestampt at which slot was last inactivated. > I think since we are already maintaining the inactive_count so better > to explicitly say this is the last invaliding time. last_inactive_at looks better, so will use that in the next version of the patch. > 2. > + /* > + * XXX: Can inactive_count of type uint64 ever overflow? It takes > + * about a half-billion years for inactive_count to overflow even > + * if slot becomes inactive for every 1 millisecond. So, using > + * pg_add_u64_overflow might be an overkill. > + */ > > Correct we don't need to use pg_add_u64_overflow for this counter. Will remove this comment in the next version of the patch. > + /* Convert to numeric. */ > + snprintf(buf, sizeof buf, UINT64_FORMAT, slot_contents.data.inactive_count); > + values[i++] = DirectFunctionCall3(numeric_in, > + CStringGetDatum(buf), > + ObjectIdGetDatum(0), > + Int32GetDatum(-1)); > > What is the purpose of doing this? I mean inactive_count is 8 byte > integer and you can define function outparameter as 'int8' which is 8 > byte integer. Then you don't need to convert int to string and then > to numeric? Nope, it's of type uint64, so reporting it as numeric is a way typically used elsewhere - see code around /* Convert to numeric. */. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
