Proposal : For Auto-Prewarm.

On Thu, Oct 27, 2016 at 5:09 PM, Mithun Cy <mithun.cy@enterprisedb.com>
wrote:

This a PostgreSQL contrib module which automatically dump all of the

blocknums

present in buffer pool at the time of server shutdown(smart and fast mode

only,

to be enhanced to dump at regular interval.) and load these blocks when

server restarts.

Sorry I forgot add warmup time performance measurement done based on this
patch. Adding now.
--
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

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...
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532) mobile: 512-569-9461

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: pgsql-hackers-owner@postgresql.org

[mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Mithun Cy
# pg_autoprewarm.

This a PostgreSQL contrib module which automatically dump all of the
blocknums present in buffer pool at the time of server shutdown(smart and
fast mode only, to be enhanced to dump at regular interval.) and load these
blocks when server restarts.

I welcome this feature! I remember pg_hibernate did this. I wonder what happened to pg_hibernate. Did you check it?

Regards
Takayuki Tsunakawa

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Oct 28, 2016 at 1:45 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.

There is not much common functionality between the two. The main job
of this feature is to dump the contents of shared buffers and reload
them at startup and it takes care for not over ridding the existing
shared buffer contents while reloading the dump file. This is
somewhat different to what prewarm provides which is to load the
relation blocks in memory or buffers, it has nothing to do with prior
shared buffer contents. So, I am not sure if it is good idea to add
it as a functionality with prewarm module. OTOH, if more people want
that way, then as such there is no harm in doing that way.

One point that seems to be worth discussing is when should the buffer
information be dumped to a file? Shall we dump at each checkpoint or
at regular intervals via auto prewarm worker or at some other time?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2016-10-28 12:59:58 +0530, Amit Kapila wrote:

On Fri, Oct 28, 2016 at 1:45 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.

There is not much common functionality between the two.

I don't really agree. For me manual and automated prewarming are pretty
closely related. Sure they have their independent usages, and not too
much code might be shared. But grouping them in the same extension seems
to make sense, it's confusing to have closely related but independent
extensions.

One point that seems to be worth discussing is when should the buffer
information be dumped to a file? Shall we dump at each checkpoint or
at regular intervals via auto prewarm worker or at some other time?

Should probably be at some regular interval - not sure if checkpoints
are the best time (or if it's even realistic to tie a bgworker to
checkpoints), since checkpoints have a significant impact on the state
of shared_buffers.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Oct 28, 2016 at 3:40 AM, Andres Freund <andres@anarazel.de> wrote:

There is not much common functionality between the two.

I don't really agree. For me manual and automated prewarming are pretty
closely related. Sure they have their independent usages, and not too
much code might be shared. But grouping them in the same extension seems
to make sense, it's confusing to have closely related but independent
extensions.

I agree that putting them together would be fine.

One point that seems to be worth discussing is when should the buffer
information be dumped to a file? Shall we dump at each checkpoint or
at regular intervals via auto prewarm worker or at some other time?

Should probably be at some regular interval - not sure if checkpoints
are the best time (or if it's even realistic to tie a bgworker to
checkpoints), since checkpoints have a significant impact on the state
of shared_buffers.

Checkpoints don't cause any buffer replacement, which I think is what
would be relevant here.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi Ashutosh,

This is a gentle reminder.

you assigned as reviewer to the current patch in the 11-2016 commitfest.
But you haven't shared your review yet. Please share your views about
the patch. This will help us in smoother operation of commitfest.

Please Ignore if you already shared your review.

Regards,
Hari Babu
Fujitsu Australia

you assigned as reviewer to the current patch in the 11-2016 commitfest.
But you haven't shared your review yet. Please share your views about
the patch. This will help us in smoother operation of commitfest.

Thanks for the reminder.

Mithun has not provided a patch addressing the comments upthread. I am
waiting for his response to those comments.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Sorry I took some time on this please find latest patch with addressed
review comments. Apart from fixes for comments I have introduced a new GUC
variable for the pg_autoprewarm "buff_dump_interval". So now we dump the
buffer pool metadata at every buff_dump_interval secs. Currently
buff_dump_interval can be set only at startup time. I did not choose to do
the dumping at checkpoint time, as it appeared these 2 things are not much
related and keeping it independent would be nice for usage. Also overhead
of any communication between them can be avoided.

On Fri, Oct 28, 2016 at 1:45 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:

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.

I have merged pg_autoprewarm module into pg_prewarm, This is just the
directory merge, Functionality merge is not possible pg_prewarm is just a
utility function with specific signature to load a specific relation at
runtime, where as pg_autoprewarm is a bgworker which dumps current buffer
pool and load it during startup time.

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).

function sort_cmp_func is for qsort so orderly comparison is needed to say
which is bigger or smaller, If we put blocknum first, we cannot decide same.

AFAICT this isn't necessary since palloc will error itself if it fails.

Fixed.

Since there's no method to change DEFAULT_DUMP_FILENAME, I would call it

what it is: DUMP_FILENAME.

Fixed.

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.

I think if we make it configurable I think I can put that check.

+ if (!avoid_dumping)
+               dump_now();
Perhaps that check should be moved into dump_now() itself...

Fixed.

--
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

On Tue, Nov 29, 2016 at 4:26 PM, Mithun Cy <mithun.cy@enterprisedb.com>
wrote:

Sorry I took some time on this please find latest patch with addressed
review comments. Apart from fixes for comments I have introduced a new GUC
variable for the pg_autoprewarm "buff_dump_interval". So now we dump the
buffer pool metadata at every buff_dump_interval secs. Currently
buff_dump_interval can be set only at startup time. I did not choose to do
the dumping at checkpoint time, as it appeared these 2 things are not much
related and keeping it independent would be nice for usage. Also overhead
of any communication between them can be avoided.

On Fri, Oct 28, 2016 at 1:45 AM, Jim Nasby <Jim.Nasby@bluetreble.com>
wrote:

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.

I have merged pg_autoprewarm module into pg_prewarm, This is just the
directory merge, Functionality merge is not possible pg_prewarm is just a
utility function with specific signature to load a specific relation at
runtime, where as pg_autoprewarm is a bgworker which dumps current buffer
pool and load it during startup time.

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).

function sort_cmp_func is for qsort so orderly comparison is needed to say
which is bigger or smaller, If we put blocknum first, we cannot decide same.

AFAICT this isn't necessary since palloc will error itself if it fails.

Fixed.

Since there's no method to change DEFAULT_DUMP_FILENAME, I would call it

what it is: DUMP_FILENAME.

Fixed.

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.

I think if we make it configurable I think I can put that check.

+ if (!avoid_dumping)
+               dump_now();
Perhaps that check should be moved into dump_now() itself...

Fixed.

Moved to next CF with "needs review" status.

Regards,
Hari Babu
Fujitsu Australia

I took a look at this again, and it doesn't appear to be working for me. The library is being loaded during startup, but I don't see any further activity in the log, and I don't see an autoprewarm file in $PGDATA.

There needs to be some kind of documentation change as part of this patch.

I'm not sure the default GUC setting of 0 makes sense. If you've loaded the module, presumably you want it to be running. I think it'd be nice if the GUC had a -1 setting that meant to use checkpoint_timeout.

Having the GUC be restart-only is also pretty onerous. I don't think it'd be hard to make the worker respond to a reload... there's code in the autovacuum launcher you could use as an example.

I'm also wondering if this really needs to be a permanently running process... perhaps the worker could simply be started as necessary? Though maybe that means it wouldn't run at shutdown. Also not sure if it could be relaunched when a reload happens.

I'm marking this as waiting on author for now, because it's not working for me and needs documentation.

The new status of this patch is: Waiting on Author

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Jan 24, 2017 at 5:07 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:

I took a look at this again, and it doesn't appear to be working for me. The library is being loaded during startup, but I don't see any further activity in the log, and I don't see an autoprewarm file in $PGDATA.

Hi Jim,
Thanks for looking into this patch, I just downloaded the patch and
applied same to the latest code, I can see file " autoprewarm.save" in
$PGDATA which is created and dumped at shutdown time and an activity
is logged as below
2017-01-24 13:22:25.012 IST [91755] LOG: Buffer Dump: saved metadata
of 59 blocks.

In my code by default, we only dump at shutdown time. If we want to
dump at regular interval then we need to set the GUC
pg_autoprewarm.buff_dump_interval to > 0. I think I am missing
something while trying to recreate the bug reported above. Can you
please let me know what exactly you mean by the library is not
working.

There needs to be some kind of documentation change as part of this patch.

Thanks, I will add a sgml for same.

For remaining suggestions, I will try to address in my next patch
based on its feasibility.

--
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Jan 24, 2017 at 1:56 PM, Mithun Cy <mithun.cy@enterprisedb.com>
wrote:

On Tue, Jan 24, 2017 at 5:07 AM, Jim Nasby <Jim.Nasby@bluetreble.com>
wrote:

I took a look at this again, and it doesn't appear to be working for me.

The library is being loaded during startup, but I don't see any further
activity in the log, and I don't see an autoprewarm file in $PGDATA.

Hi Jim,
Thanks for looking into this patch, I just downloaded the patch and
applied same to the latest code, I can see file " autoprewarm.save" in
$PGDATA which is created and dumped at shutdown time and an activity
is logged as below
2017-01-24 13:22:25.012 IST [91755] LOG: Buffer Dump: saved metadata
of 59 blocks.

In my code by default, we only dump at shutdown time. If we want to
dump at regular interval then we need to set the GUC
pg_autoprewarm.buff_dump_interval to > 0. I think I am missing
something while trying to recreate the bug reported above. Can you
please let me know what exactly you mean by the library is not
working.

There needs to be some kind of documentation change as part of this

patch.
Thanks, I will add a sgml for same.

For remaining suggestions, I will try to address in my next patch
based on its feasibility.

The patch works for me too.

Few initial comments:

1. I think the README was maintained as is from the 1st version and says
pg_autoprewarm is a contrib module. It should be rewritten to
pg_autoprewarm is a part of pg_prewarm module. The documentation should be
added to pgprewarm.sgml instead of the README

2. buff_dump_interval could be renamed to just dump_interval or
buffer_dump_interval. Also, since it is now part of pg_prewarm. I think it
makes sense to have the conf parameter be: pg_prewarm.xxx instead of
pg_autoprewarm.xxx

3. During startup we get the following message:

2017-01-24 16:13:57.615 IST [90061] LOG: Num buffers : 272

I could be better written as “pg_prewarm: 272 blocks loaded from dump” or
something similar.

4. Also, the message while dumping says:

2017-01-24 16:15:17.712 IST [90061] LOG: Buffer Dump: saved metadata of
272 blocks

It would be better to write the module name in message instead of "Buffer
Dump"

Thank you,

Beena Emerson

Have a Great Day!

On Fri, Oct 28, 2016 at 6:36 AM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:

I welcome this feature! I remember pg_hibernate did this. I wonder what happened to pg_hibernate. Did you check it?

Thanks, when I checked with pg_hibernate there were two things people
complained about it. Buffer loading will start after the recovery is
finished and the database has reached the consistent state. Two It can
replace existing buffers which are loaded due to recovery and newly
connected clients. And this solution tried to solve them.

--
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 1/24/17 4:56 AM, Beena Emerson wrote:

2. buff_dump_interval could be renamed to just dump_interval or
buffer_dump_interval. Also, since it is now part of pg_prewarm. I think
it makes sense to have the conf parameter be: pg_prewarm.xxx instead of
pg_autoprewarm.xxx

I'd really like to find terminology other than "buffer dump", because
that makes it sound like we're dumping the contents of the buffers
themselves.

Maybe block_map? Buffer_map?
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 1/24/17 2:26 AM, Mithun Cy wrote:

Thanks for looking into this patch, I just downloaded the patch and
applied same to the latest code, I can see file " autoprewarm.save" in
$PGDATA which is created and dumped at shutdown time and an activity
is logged as below
2017-01-24 13:22:25.012 IST [91755] LOG: Buffer Dump: saved metadata
of 59 blocks.

Yeah, I wasn't getting that at all, though I did see the shared library
being loaded. If I get a chance I'll try it again.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Jan 25, 2017 at 10:36 AM, Jim Nasby <Jim.Nasby@bluetreble.com>
wrote:

On 1/24/17 2:26 AM, Mithun Cy wrote:

Thanks for looking into this patch, I just downloaded the patch and
applied same to the latest code, I can see file " autoprewarm.save" in
$PGDATA which is created and dumped at shutdown time and an activity
is logged as below
2017-01-24 13:22:25.012 IST [91755] LOG: Buffer Dump: saved metadata
of 59 blocks.

Yeah, I wasn't getting that at all, though I did see the shared library
being loaded. If I get a chance I'll try it again.

Hope u added the following to the conf file:

shared_preload_libraries = 'pg_autoprewarm' # (change requires restart)
pg_autoprewarm.buff_dump_interval=20

Even after this u could not see the message then that's strange.

--
Thank you,

Beena Emerson

Have a Great Day!

On Tue, Jan 24, 2017 at 5:07 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:

I took a look at this again, and it doesn't appear to be working for me. The library is being loaded during startup, but I don't see any further activity in the log, and I don't see an autoprewarm file in $PGDATA.

There needs to be some kind of documentation change as part of this patch.

I'm not sure the default GUC setting of 0 makes sense. If you've loaded the module, presumably you want it to be running. I think it'd be nice if the GUC had a -1 setting that meant to use checkpoint_timeout.

Having the GUC be restart-only is also pretty onerous. I don't think it'd be hard to make the worker respond to a reload... there's code in the autovacuum launcher you could use as an example.

+1. I don't think there should be any problem in making it PGC_SIGHUP.

I'm also wondering if this really needs to be a permanently running process... perhaps the worker could simply be started as necessary?

Do you want to invoke worker after every buff_dump_interval? I think
that will be bad in terms of starting a new process and who will
monitor when to start such a process. I think it is better to keep it
as a permanently running background process if loaded by user.

Though maybe that means it wouldn't run at shutdown.

Yeah, that will be another drawback.

Few comments found while glancing the patch.

1.
+TO DO:
+------
+Add functionality to dump based on timer at regular interval.

I think you need to remove above TO DO.

2.
+ /* Load the page only if there exist a free buffer. We do not want to
+ * replace an existing buffer. */

This is not a PG style multiline comment.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers