New XLOG record indicating WAL-skipping

On Wed, Dec 9, 2009 at 6:25 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> Here is the patch:
>
> - Write an XLOG UNLOGGED record in WAL if WAL-logging is skipped for only
>  the reason that WAL archiving is not enabled and such record has not been
>  written yet.
>
> - Cause archive recovery to end if an XLOG UNLOGGED record is found during
>  it.

Here's an updated version of my "New XLOG record indicating WAL-skipping" patch.
http://archives.postgresql.org/pgsql-hackers/2009-12/msg00788.php

This is rebased to CVS HEAD.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Fujii Masao wrote:
> On Wed, Dec 9, 2009 at 6:25 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> Here is the patch:
>>
>> - Write an XLOG UNLOGGED record in WAL if WAL-logging is skipped for only
>> the reason that WAL archiving is not enabled and such record has not been
>> written yet.
>>
>> - Cause archive recovery to end if an XLOG UNLOGGED record is found during
>> it.
>
> Here's an updated version of my "New XLOG record indicating WAL-skipping" patch.
> http://archives.postgresql.org/pgsql-hackers/2009-12/msg00788.php

Thanks!

I don't like special-casing UNLOGGED records in XLogInsert and
ReadRecord(). Those functions are complicated enough already. The
special handling from XLogInsert() (and a few other places) is only
required because the UNLOGGED records carry no payload. That's easy to
avoid, just add some payload to them, doesn't matter what it is. And I
don't think ReadRecord() is the right place to emit the errors/warnings,
that belongs naturally in xlog_redo().

It might be useful to add some information in the records telling why
WAL-logging was skipped. It might turn out to be useful in debugging.
That also conveniently adds payload to the records, to avoid the
special-casing in XLogInsert() :-).

I think it's a premature optimization to skip writing the records if
we've written in the same session already. Especially with the 'reason'
information added to the records, it's nice to have a record of each
such operation. All operations that skip WAL-logging are heavy enough
that an additional WAL record will make no difference. I can see that it
was required to avoid the flooding from heap_insert(), but we can move
the XLogSkipLogging() call from heap_insert() to heap_sync().

Attached is an updated patch, doing the above. Am I missing anything?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Fri, Jan 15, 2010 at 11:28 AM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> I can see that it
> was required to avoid the flooding from heap_insert(), but we can move
> the XLogSkipLogging() call from heap_insert() to heap_sync().
>
> Attached is an updated patch, doing the above. Am I missing anything?

Hm, perhaps the timing is actually important? What if someone takes a
hot backup while an unlogged operation is in progress. The checkpoint
can occur and finish and the backup finish all while the unlogged
operation is happening. Then the replica can start restoring archived
logs from that point forward. In the original coding it sounds like
the replica would never notice the unlogged operation which might not
have been synced before the start of the initial hot backup. If the
record occurs when the sync begins then the replica would be in
trouble if the checkpoint begins before the operation completed but
finished after the sync began and the record was emitted.

It seems like it's important that the record occur only after the sync
*completes* to be sure that if the replica doesn't see the record then
it knows the sync was done before its initial backup image was taken.

--
greg

Greg Stark wrote:
> What if someone takes a hot backup while an unlogged operation is in progress.

Can't do that, pg_start_backup() throws an error if archive_mode=off.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Fri, Jan 15, 2010 at 8:28 PM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> I don't like special-casing UNLOGGED records in XLogInsert and
> ReadRecord(). Those functions are complicated enough already. The
> special handling from XLogInsert() (and a few other places) is only
> required because the UNLOGGED records carry no payload. That's easy to
> avoid, just add some payload to them, doesn't matter what it is. And I
> don't think ReadRecord() is the right place to emit the errors/warnings,
> that belongs naturally in xlog_redo().
>
> It might be useful to add some information in the records telling why
> WAL-logging was skipped. It might turn out to be useful in debugging.
> That also conveniently adds payload to the records, to avoid the
> special-casing in XLogInsert() :-).
>
> I think it's a premature optimization to skip writing the records if
> we've written in the same session already. Especially with the 'reason'
> information added to the records, it's nice to have a record of each
> such operation. All operations that skip WAL-logging are heavy enough
> that an additional WAL record will make no difference. I can see that it
> was required to avoid the flooding from heap_insert(), but we can move
> the XLogSkipLogging() call from heap_insert() to heap_sync().
>
> Attached is an updated patch, doing the above. Am I missing anything?

Thanks a lot! Your change seems to be OK.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Sat, Jan 16, 2010 at 3:16 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> Attached is an updated patch, doing the above. Am I missing anything?
>
> Thanks a lot! Your change seems to be OK.

We'll need to do some more work after the following patch
has been committed.
http://archives.postgresql.org/pgsql-hackers/2010-01/msg01715.php

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Fri, 2010-01-15 at 13:28 +0200, Heikki Linnakangas wrote:
> I think it's a premature optimization to skip writing the records if
> we've written in the same session already. Especially with the
> 'reason'
> information added to the records, it's nice to have a record of each
> such operation. All operations that skip WAL-logging are heavy enough
> that an additional WAL record will make no difference. I can see that
> it
> was required to avoid the flooding from heap_insert(), but we can move
> the XLogSkipLogging() call from heap_insert() to heap_sync().

Can we call that XLogReportUnloggedStatement() or similar?
XlogSkipLogging() sounds like a request rather than a mark/report/record
type of action.

> Attached is an updated patch, doing the above. Am I missing anything?

Sounds OK and works with Hot Standby.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs wrote:
> Can we call that XLogReportUnloggedStatement() or similar?
> XlogSkipLogging() sounds like a request rather than a mark/report/record
> type of action.

Agreed. I vote for XLogReportUnloggedOperation(). I'll change it to that
before committing, unless Fujii beats me to it.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Mon, Jan 18, 2010 at 9:17 PM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> Agreed. I vote for XLogReportUnloggedOperation().

OK.

> I'll change it to that
> before committing, unless Fujii beats me to it.

Yeah, please go ahead.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center