RecordTransactionCommit() and SharedInvalidationMessages

It appears to me that RecordTransactionCommit() only needs to WAL-log
shared invalidation messages when wal_level is hot_standby, but I
don't see a guard to prevent it from doing it in all cases. Am I
missing something?

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

On Tue, Aug 10, 2010 at 9:30 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> It appears to me that RecordTransactionCommit() only needs to WAL-log
> shared invalidation messages when wal_level is hot_standby, but I
> don't see a guard to prevent it from doing it in all cases.

Perhaps right. During not hot standby, there is no backend which the
startup process should send invalidation message to in the standby.
So, ISTM we don't need to log invalidation message when wal_level is
not hot_standby.

Regards,

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

On Wed, Aug 11, 2010 at 1:17 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Tue, Aug 10, 2010 at 9:30 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> It appears to me that RecordTransactionCommit() only needs to WAL-log
>> shared invalidation messages when wal_level is hot_standby, but I
>> don't see a guard to prevent it from doing it in all cases.
>
> Perhaps right. During not hot standby, there is no backend which the
> startup process should send invalidation message to in the standby.
> So, ISTM we don't need to log invalidation message when wal_level is
> not hot_standby.

The fix looks pretty simple (see attached), although I don't have any
clear idea how to test it. I guess the question is whether we should
back-patch this to 9.0. It isn't technically necessary for
correctness, but the whole point of introducing the wal_level GUC was
to insulate people not running Hot Standby from possible bugs in the
Hot Standby code, as well as to avoid unnecessary WAL bloat, so on
balance I'm inclined to think we should go ahead and back-patch it.

Other opinions?

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

On 11/08/10 16:46, Robert Haas wrote:
> On Wed, Aug 11, 2010 at 1:17 AM, Fujii Masao<masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Tue, Aug 10, 2010 at 9:30 AM, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
>>> It appears to me that RecordTransactionCommit() only needs to WAL-log
>>> shared invalidation messages when wal_level is hot_standby, but I
>>> don't see a guard to prevent it from doing it in all cases.
>>
>> Perhaps right. During not hot standby, there is no backend which the
>> startup process should send invalidation message to in the standby.
>> So, ISTM we don't need to log invalidation message when wal_level is
>> not hot_standby.
>
> The fix looks pretty simple (see attached), although I don't have any
> clear idea how to test it.

Should use XLogStandbyInfoActive() macro, for the sake of consistency.

> I guess the question is whether we should
> back-patch this to 9.0. It isn't technically necessary for
> correctness, but the whole point of introducing the wal_level GUC was
> to insulate people not running Hot Standby from possible bugs in the
> Hot Standby code, as well as to avoid unnecessary WAL bloat, so on
> balance I'm inclined to think we should go ahead and back-patch it.

+1 for backpatching. Keeping the branches closer to each other makes
backporting any future fixes easier too.

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

On Wed, Aug 11, 2010 at 11:35 PM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> On 11/08/10 16:46, Robert Haas wrote:
>>
>> On Wed, Aug 11, 2010 at 1:17 AM, Fujii Masao<masao(dot)fujii(at)gmail(dot)com>
>>  wrote:
>>>
>>> On Tue, Aug 10, 2010 at 9:30 AM, Robert Haas<robertmhaas(at)gmail(dot)com>
>>>  wrote:
>>>>
>>>> It appears to me that RecordTransactionCommit() only needs to WAL-log
>>>> shared invalidation messages when wal_level is hot_standby, but I
>>>> don't see a guard to prevent it from doing it in all cases.
>>>
>>> Perhaps right. During not hot standby, there is no backend which the
>>> startup process should send invalidation message to in the standby.
>>> So, ISTM we don't need to log invalidation message when wal_level is
>>> not hot_standby.
>>
>> The fix looks pretty simple (see attached), although I don't have any
>> clear idea how to test it.
>
> Should use XLogStandbyInfoActive() macro, for the sake of consistency.

And, RelcacheInitFileInval should be initialized with false just in case.

>> I guess the question is whether we should
>> back-patch this to 9.0.  It isn't technically necessary for
>> correctness, but the whole point of introducing the wal_level GUC was
>> to insulate people not running Hot Standby from possible bugs in the
>> Hot Standby code, as well as to avoid unnecessary WAL bloat, so on
>> balance I'm inclined to think we should go ahead and back-patch it.
>
> +1 for backpatching. Keeping the branches closer to each other makes
> backporting any future fixes easier too.

+1

Regards,

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

On Thu, Aug 12, 2010 at 12:11 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>>> It appears to me that RecordTransactionCommit() only needs to WAL-log
>>>>> shared invalidation messages when wal_level is hot_standby, but I
>>>>> don't see a guard to prevent it from doing it in all cases.
[...]
>>> The fix looks pretty simple (see attached), although I don't have any
>>> clear idea how to test it.
>> Should use XLogStandbyInfoActive() macro, for the sake of consistency.
> And, RelcacheInitFileInval should be initialized with false just in case.

How about this?

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

On Fri, Aug 13, 2010 at 10:24 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Aug 12, 2010 at 12:11 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>>>> It appears to me that RecordTransactionCommit() only needs to WAL-log
>>>>>> shared invalidation messages when wal_level is hot_standby, but I
>>>>>> don't see a guard to prevent it from doing it in all cases.
> [...]
>>>> The fix looks pretty simple (see attached), although I don't have any
>>>> clear idea how to test it.
>>> Should use XLogStandbyInfoActive() macro, for the sake of consistency.
>> And, RelcacheInitFileInval should be initialized with false just in case.
>
> How about this?

Looks good to me.

Regards,

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

On Thu, Aug 12, 2010 at 9:43 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Fri, Aug 13, 2010 at 10:24 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Aug 12, 2010 at 12:11 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>>>>> It appears to me that RecordTransactionCommit() only needs to WAL-log
>>>>>>> shared invalidation messages when wal_level is hot_standby, but I
>>>>>>> don't see a guard to prevent it from doing it in all cases.
>>>> Should use XLogStandbyInfoActive() macro, for the sake of consistency.
>>> And, RelcacheInitFileInval should be initialized with false just in case.
>>
>> How about this?
>
> Looks good to me.

Committed.

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