From: | Adrien NAYRAT <adrien(dot)nayrat(at)anayrat(dot)info> |
---|---|
To: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, "Kuroda, Hayato" <kuroda(dot)hayato(at)jp(dot)fujitsu(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Log a sample of transactions |
Date: | 2019-03-27 09:22:20 |
Message-ID: | ba297447-f734-81d1-f6d9-8405f38eb90f@anayrat.info |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hello,
On 3/27/19 7:05 AM, Masahiko Sawada wrote:
>>>> Why are both of these checked? ISTM once xact_is_sampled is set, we
>>>> ought not to respect a different value of log_xact_sample_rate for the
>>>> rest of that transaction.
>>> I added theses checks to allow to disable logging during a sampled
>>> transaction, per suggestion of Masahiko Sawada:
>>> https://www.postgresql.org/message-id/CAD21AoD4t%2BhTV6XfK5Yz%3DEocB8oMyJSYFfjAryCDYtqfib2GrA%40mail.gmail.com
>> I added a comment to explain why transaction logging is rechecked.
> Umm, I'm inclined to think that we should not disable logging within
> the transaction. If we allow that, since users can disable the logging
> within the transaction by setting it to 0 they may think that we can
> change the rate during the transaction, which is wrong. If we want
> this behavior we should document it but we can make user not being
> able to change the rate during the transaction to avoid confusion. And
> the latter looks more understandable and straightforward. This is
> different comment what I did before, I'm sorry for confusing you.
Don't worry, I will change that.
>
>>>> As far as I can tell xact_is_sampled is not properly reset in case of
>>>> errors?
>>>>
>> I am not sure if I should disable logging in case of errors. Actually we
>> have:
>>
>> postgres=# set log_transaction_sample_rate to 1;
>> SET
>> postgres=# set client_min_messages to 'LOG';
>> LOG: duration: 0.392 ms statement: set client_min_messages to 'LOG';
>> SET
>> postgres=# begin;
>> LOG: duration: 0.345 ms statement: begin;
>> BEGIN
>> postgres=# select 1;
>> LOG: duration: 0.479 ms statement: select 1;
>> ?column?
>> ----------
>> 1
>> (1 row)
>>
>> postgres=# select * from missingtable;
>> ERROR: relation "missingtable" does not exist
>> LINE 1: select * from missingtable;
>> ^
>> postgres=# select 1;
>> ERROR: current transaction is aborted, commands ignored until end of
>> transaction block
>> postgres=# rollback;
>> LOG: duration: 11390.295 ms statement: rollback;
>> ROLLBACK
>>
>> If I reset xact_is_sampled (after the first error inside
>> AbortTransaction if I am right), "rollback" statement will not be
>> logged. I wonder if this "statement" should be logged?
>>
>> If the answer is no, I will reset xact_is_sampled in AbortTransaction.
>>
> FWIW, I'd prefer to log "rollback" as well so as user can recognize
> the end of transaction.
Ok.
>
> + /* Determine if statements are logged in this transaction */
> + xact_is_sampled = random() <= log_xact_sample_rate * MAX_RANDOM_VALUE;
>
> If log_xact_sample_rate is 1 we always log all statement in the
> transaction regardless of value of random(). Similarly if it's 0, we
> never log. I think we can avoid unnecessary random() call in both case
> like log_statement_sample_rate does.
I agree, I will change that.
>
> {"log_statement_sample_rate", PGC_SUSET, LOGGING_WHEN,
> gettext_noop("Fraction of statements over
> log_min_duration_statement to log."),
> gettext_noop("If you only want a sample, use a
> value between 0 (never "
> - "log) and 1.0 (always log).")
> + "log) and 1 (always log).")
> },
> &log_statement_sample_rate,
> 1.0, 0.0, 1.0,
> NULL, NULL, NULL
> },
>
> This change is not relevant with log_transaction_sample_rate feature
> but why do we need this change? In postgresql.conf.sample the
> description of both log_statement_sample_rate and
> log_transaction_sample_rate use 1.0 instead of 1, like "1.0 logs all
> statements from all transactions, 0 never logs". If we need this
> change I think it should be a separate patch.
Sorry, I changed that, someone suggest using either "0" and "1", or
"0.0" and "1.0" but not mixing both. I will remove this change.
Thanks for your review.
From | Date | Subject | |
---|---|---|---|
Next Message | Fabien COELHO | 2019-03-27 09:36:48 | Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd) |
Previous Message | Thomas Munro | 2019-03-27 08:49:45 | Re: txid_status() off-by-one error |