On Fri, Oct 28, 2016 at 5:15 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
> On 10/27/16 6:39 AM, Mithun Cy wrote:
>>
>> # pg_autoprewarm.
>
>
> IMO it would be better to add this functionality to pg_prewarm instead of
> creating another contrib module. That would reduce confusion and reduce the
> amount of code necessary.
>
> + cmp_member_elem(database);
> + cmp_member_elem(spcNode);
> + cmp_member_elem(filenode);
> + cmp_member_elem(forknum);
> + cmp_member_elem(blocknum);
>
> Presumably the first 4 numbers will vary far less than blocknum, so it's
> probably worth reversing the order here (or at least put blocknum first).
>
> I didn't look at the two load functions since presumably they'd go
> away/change significantly if this was combined with pg_prewarm.
>
> + if (!block_info_array)
> + elog(ERROR, "Out of Memory!");
> AFAICT this isn't necessary since palloc will error itself if it fails.
>
> + snprintf(transient_dump_file_path, sizeof(dump_file_path),
> + "%s.save.tmp", DEFAULT_DUMP_FILENAME);
> Since there's no method to change DEFAULT_DUMP_FILENAME, I would call it
> what it is: DUMP_FILENAME.
>
> Also, maybe worth an assert to make sure there was enough room for the
> complete filename. That'd need to be a real check if this was configurable
> anyway.
>
> + if (!avoid_dumping)
> + dump_now();
> Perhaps that check should be moved into dump_now() itself...
As this picked up my curiosity...
+/*
+ * ReadBufferForPrewarm -- This new interface is for pg_autoprewarm.
+ */
+Buffer
+ReadBufferForPrewarm(SMgrRelation smgr, char relpersistence,
+ ForkNumber forkNum, BlockNumber blockNum,
+ ReadBufferMode mode, BufferAccessStrategy strategy)
+{
+ bool hit;
+
+ return ReadBuffer_common(smgr, relpersistence, forkNum, blockNum,
+ mode, strategy, &hit);
+}
May be better to just expose ReadBuffer_common or rename it.
--
Michael