Hello
It is something like
rmgr: Heaplen (rec/tot):143/ 143, tx:748, lsn:
0/01530AF0, prev 0/01530AB8, desc: INSERT off: 17, flags: 0x00, blkref #0: rel
1664/0/1260 blk 0
just insertion into pg_authid (oid=1260)
regards, Sergei
Hello
auto_explain.log_level is available since postgresql 12.
postgres=# load 'auto_explain';
LOAD
postgres=# set auto_explain.log_min_duration to 0;
SET
postgres=# set auto_explain.log_level to 'notice';
SET
postgres=# select 1;
NOTICE: duration: 0.010 ms plan:
Query Text: select 1;
Result
Hello!
I noticed that the same block
-- SET statements.
-- These use two different strings, still they count as one entry.
SET work_mem = '1MB';
Set work_mem = '1MB';
SET work_mem = '2MB';
RESET work_mem;
SET enable_seqscan = off;
SET enable_seqscan = on;
RESET enable_seqscan;
is checked twice
Hello
I encountered a very lucky logical decoding error on the publisher:
2023-09-05 09:58:38.955 UTC 28316 melkij@postgres from [local] [vxid:3/0
txid:0] [START_REPLICATION] LOG: starting logical decoding for slot "pubsub"
2023-09-05 09:58:38.955 UTC 28316 melkij@postgres from [local]
Hello
I think it's appropriate to add on the restrictions page. (But mentioning that
this restriction is only for subscriber)
If the list were larger, then the restrictions page could be divided into
publisher and subscriber restrictions. But not for one very specific
restriction.
regards,
Hello
My +1 to have such a function in core or in some contrib at least (pg_surgery?
amcheck?).
In the past, more than once I needed to find a damaged tuple knowing only chunk
id and toastrelid. This feature would help a lot.
regards, Sergei
>> Is this restriction only for the subscriber?
>>
>> If we have not changed the replica identity and there is no primary key,
>> then we forbid update and delete on the publication side (a fairly common
>> usage error at the beginning of using publications).
>> If we have replica identity FULL
Hello
Is this restriction only for the subscriber?
If we have not changed the replica identity and there is no primary key, then
we forbid update and delete on the publication side (a fairly common usage
error at the beginning of using publications).
If we have replica identity FULL (the table
Hello!
The documentation still describes the function pg_stat_statements_reset like
this
> By default, this function can only be executed by superusers.
But unfortunately, this part was lost somewhere.
-- Don't want this to be available to non-superusers.
REVOKE ALL ON FUNCTION
Hello!
Unfortunately, rebase is needed again due to recent changes in queryjumblefuncs
( 9ba37b2cb6a174b37fc51d0649ef73e56eae27fc )
It seems a little strange to me that with const_merge_threshold = 1, such a
test case gives the same result as with const_merge_threshold = 2
select
Hello
Do we need new syntax actually? I think that a global unique index can be
created automatically instead of raising an error "unique constraint on
partitioned table must include all partitioning columns"
regards, Sergei
Hi
Ubuntu 16.04 is EOL from April 2021, over a year ago.
https://wiki.postgresql.org/wiki/Apt/FAQ#Where_are_older_versions_of_the_packages.3F
regards, Sergei
Hello!
Unfortunately the patch needs another rebase due to the recent split of guc.c
(0a20ff54f5e66158930d5328f89f087d4e9ab400)
I'm reviewing a patch on top of a previous commit and noticed a failed test:
# Failed test 'no parameters missing from postgresql.conf.sample'
# at
Hello
Yes, I already wrote about this artifact. And created CF app entry so it
wouldn't get lost: https://commitfest.postgresql.org/38/3616/
regards, Sergei
Hello
select array_agg(distinct mycolumn) from
from the very beginning? Even the 7.1 manual describes such a syntax:
https://www.postgresql.org/docs/7.1/sql-expressions.html
regards, Sergei
Hello
I just spotted in src/include/access/xlog.h:
extern XLogRecPtr CalculateMaxmumSafeLSN(void);
This function doesn't seem to be used anywhere or even defined? "git grep
CalculateMaxmumSafeLSN" shows only this line.
Was added in c6550776394e25c1620bc8258427c8f1d448080d "Allow users to limit
Hello
Thank you for the explanation!
> unreadable HTML mess
ouch, sorry. "Nobody uses plain text mail, we dropped this thing in the
interface” said yandex team. (I know that some members of the Yandex team read
mailing lists, could you ping your colleagues?)
> but I'm not convinced we want
Hello I have such case: create table test (id int not null, status text);insert into test select i, 'foo' from generate_series(1,100) i;update test set status = 'bar' where id <= 10;create index test_id on test (id );create index test_status_partial on test (status) where status =
Hello postgresql uses hba_file configuration parameter: https://www.postgresql.org/docs/current/runtime-config-file-locations.htmlSo could be changed in postgresql.conf regards, Sergei
Hello
Thanks, I missed this thread.
> +CHECKPOINT { 'fast' | 'spread'
> }
Unpaired tag in docs.
That was all I noticed in 0001. Still not sure where is the difference between
"change NOWAIT to WAIT" and "change NOWAIT to something else descriptive". But
fine, I can live with WAIT.
Hello
I found that in 0001 you propose to rename few options. Probably we could
rename another option for clarify? I think FAST (it's about some bw limits?)
and WAIT (wait for what? checkpoint?) option names are confusing.
Could we replace FAST with "CHECKPOINT [fast|spread]" and WAIT to
Hello
I build gcc version 12.0.0 20210603 (experimental) locally, then tried
REL_13_STABLE with same CFLAGS as serinus
./configure --prefix=/home/melkij/tmp/pgdev/inst CFLAGS="-O3 -ggdb -g3 -Wall
-Wextra -Wno-unused-parameter -Wno-sign-compare
-Wno-missing-field-initializers"
I noticed that the fix has been committed, thank you!
regards, Sergei
Hello
In src/backend/utils/adt/genfile.c in pg_read_file we have errhint
> errhint("Consider using %s, which is part of core, instead.",
>"pg_file_read()")
Shouldn't pg_read_file() be written here?
regards, Sergei
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation:tested, passed
Hello
Look good for me. I think the patch is ready for commiter.
Hello
In commit 898e5e3290a72d288923260143930fb32036c00c [1] we lowered the lock
level on the parent relation. I found in discussion [2]:
> David Rowley recently pointed out that we can modify
> CREATE TABLE .. PARTITION OF to likewise not obtain AEL anymore.
> Apparently it just requires
Hello
Seems we need also change PGSS_FILE_HEADER.
regards, Sergei
Hello
> To get an increase in the number of records that means that the same
> statement
> would appear at top level AND nested level. This seems a corner case with
> very low
> (neglectible) occurence rate.
+1
I think splitting fields into plans_toplevel / plans_nested will be less
convenient.
Hello
> - add a parent_statement_id column that would be NULL for top level queries
Will generate too much entries... Every FK for each different delete/insert,
for example.
But very useful for databases with a lot of stored procedures to find where
this query is called. May be new mode track
Hello
> OK, so I pushed the patch. Thanks!
Thank you!
regards, Sergei
Hello
> First of all, I confirmed the server started up without this patch.
It is possible only with manually configured hot_standby = off, right?
We have ERROR in hot standby mode just below in CheckRequiredParameterValues.
regards, Sergei
Hello
> I think I like "unpaused" better here, because "resumed" would seem to
> imply that recovery can actually continue.
Good, I agree.
> One thing that has not been added to my patch is the equivalent of
> 496ee647ecd2917369ffcf1eaa0b2cdca07c8730, which allows promotion while
> recovery is
Hello
Thank you! I'm on vacation, so I was finally able to review the patch.
Seems WAIT_EVENT_RECOVERY_PAUSE addition was lost during patch simplification.
> ereport(FATAL,
> (errmsg("recovery aborted because of
> insufficient parameter settings"),
>
Hello
I found a simple test case:
CREATE TABLE test(id int NOT NULL, gen int GENERATED ALWAYS AS (id + 1) STORED)
partition by range (id);
create table test_part_create partition of test for values from ( 0) to (10);
create table test_part_attach (id int NOT NULL, gen int);
alter table test
Hello
> Anyway, for now I think that your first patch would be enough, i.e.,
> just change the context of restore_command to PGC_SIGHUP.
Glad to hear. Attached a rebased version of the original proposal.
regards, Sergeidiff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index
Hello
> Even if PITR is commanded, crash recovery can run before starting
> archive recovery if the server was not gracefully shut down.
Hmm... Still not sure how it's possible. Both readRecoverySignalFile and
validateRecoveryParameters are called early in StartupXLOG. If PITR was
commanded -
Hello
> If someone changes restore_command to '' then reload while crash
> recovery is running, the server stops for no valid reason.
While *crash* recovery is running? It's possible only during Point-in-Time
Recovery, no?
At the beginning of validateRecoveryParameters we check
Hello
> I'm wondering if it's safe to allow restore_command to be emptied during
> archive recovery. Even when it's emptied, archive recovery can proceed
> by reading WAL files from pg_wal directory. This is the same behavior as
> when restore_command is set to, e.g., /bin/false.
I am always
Hello
> Currently when restore_command is not set, archive recovery fails
> at the beginning. With the patch, how should we treat the case where
> retore_command is reset to empty during archive recovery? We should
> reject that change of restore_command?
Good point. I think we should reject
Hello
Sorry for late response.
>> > ... but what's the corresponding hazard here, exactly? It doesn't seem
>> > that there's any way in which the decision one process makes affects
>> > the decision the other process makes. There's still a race condition:
>> > it's possible for a walsender
Hello
Yep, I think it's useful and I already posted patch in this thread:
https://www.postgresql.org/message-id/flat/3090621585393698%40myt5-1466095fe4e5.qloud-c.yandex.net#ee6574e93982b5628d140f15cb44
Currently without consensus
regards, Sergei
LE ... SET NOT NULL to avoid unnecessary table scans (Sergei
> Kornilov)
> This can be optimized when the table's column constraints can be recognized
> as disallowing nulls.
> - Do you know if there are any blog posts etc. discussing this? (I'm
> definitely going to write one
Hello
Correct index lookup is a difficult task. I tried to implement this
previously...
But the answer in SO is a bit incomplete for recent postgresql releases.
Seqscan is not the only possible way to set not null in pg12+. My patch was
commited ( https://commitfest.postgresql.org/22/1389/ )
Hello
Yes, this is expected. Walreceiver always start streaming from beginning of the
wal segment.
./src/backend/replication/walreceiverfuncs.c in RequestXLogStreaming:
* We always start at the beginning of the segment. That prevents a
broken
* segment (i.e., with no records
Hello
> mostly in an aggregation to show
> in a compact way what items were grouped together.
Aggregate functions have syntax for ordering: just "select array_agg(i order by
i) from "
Described here:
https://www.postgresql.org/docs/current/sql-expressions.html#SYNTAX-AGGREGATES
regards,
My proposal does not change the behavior after this commit, only
changing the lines in the logs.
>>>
>>> Yes. What's your opinion about the latest patch based on Robert's idea?
>>> Barring any ojection, I'd like to commit that.
>>
>> Oh, sorry. Looks good and solves my issue
>
>
Hello
>> My proposal does not change the behavior after this commit, only changing
>> the lines in the logs.
>
> Yes. What's your opinion about the latest patch based on Robert's idea?
> Barring any ojection, I'd like to commit that.
Oh, sorry. Looks good and solves my issue
regards, Sergei
Hello
> When I test the patch, I find an issue: I start a stream with
> 'promote_trigger_file'
> GUC valid, and exec pg_wal_replay_pause() during recovery and as below it
> shows success to pause at the first time. I think it use a initialize
> 'SharedPromoteIsTriggered' value first time I exec
Hello
I want to return to this discussion, since primary_conninfo is now PGC_SIGHUP
(and I hope will not be reverted)
> On 2019-02-08 09:19:31 +0900, Michael Paquier wrote:
>> On Thu, Feb 07, 2019 at 11:06:27PM +0100, Peter Eisentraut wrote:
>> > Probably right. I figured it would be useful
Hello
Thank you very much!
I attached updated patch: walreceiver will use configured primary_slot_name as
temporary slot name if wal_receiver_create_temp_slot is enabled.
regards, Sergeidiff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 2de21903a1..8983cb5f5e 100644
---
Hello
Thank you!
> I think I should set aside your new draft for now
I agree, this patch definitely needs a bit more time to review. (currently it
applies on top of v13 patch cleanly)
> but I think we should still get it in pg13 to avoid having the change the
> semantics of the
> walreceiver
Hello
> I think we can set wait event WAIT_EVENT_RECOVERY_PAUSE here.
+1, since we added this in recoveryPausesHere.
PS: do we need to add a prototype for the RecoveryRequiredIntParameter function
in top of xlog.c?
regards, Sergei
Hello
In other words, patches in reverse order:
0001 will change primary_conninfo and primary_slot_name to reloadable
0002 will move wal_receiver_create_temp_slot logic to startup process (without
changing to PGC_POSTMASTER)
0003 is new draft patch: wal_receiver_create_temp_slot will use the
Hello
> I realized that the reason the tests broke after Sergei's patch is that
> recovery/t/001_stream_rep.pl's get_slot_xmins() is broken for temp
> walreceiver slots, since it's using the non-temp name it tries to give
> to the slot, rather than the temp name under which it is actually
>
Hello
> If we don't ignore walreceiver and does try to connect to the master,
> a promotion and recovery cannot end forever since new WAL data can
> be streamed. You think this behavior is more consistent?
We have no simple point to stop replay.
Well, except for "immediately" - just one easy
Hello
Thank you! You were ahead of me. I wanted to double-check my changes this
morning before posting.
> Sergei included LOG messages to indicate which setting has been changed.
> I put these in "#if 0" for now, but I'm thinking to remove these
> altogether;
No objections.
> Not sure if we
Hello
> WAL usage patch [1] increments this version to 1_4 instead of 1_8.
> I *guess* that's because previously this version was maintained
> independently from pg_stat_statements' version. For example,
> pg_stat_statements 1.4 seems to have used PGSS_V1_3.
As far as I remember, this was my
Hi
>> Could we add a few words in func.sgml to clarify the behavior? Especially
>> for users from my example above. Something like:
>>
>>> If a promotion is triggered while recovery is paused, the paused state
>>> ends, replay of any WAL immediately available in the archive or in pg_wal
>>>
Hello
> I pushed the latest version of the patch. If you have further opinion
> about immediate promotion, let's keep discussing that!
Thank you!
Honestly, I forgot that the promotion is documented in high-availability.sgml
as:
> Before failover, any WAL immediately available in the archive
Hello
> You meant that the promotion request should cause the recovery
> to finish immediately even if there are still outstanding WAL records,
> and cause the standby to become the master?
Oh, I get your point. But yes, I expect that in case of promotion request
during a pause, the user (me
Hello
(I am trying to find an opportunity to review this patch...)
Consider test case with streaming replication:
on primary: create table foo (i int);
on standby:
postgres=# select pg_wal_replay_pause();
pg_wal_replay_pause
-
(1 row)
postgres=# select
Hello
I was inactive for a while... Sorry.
>> BTW, I recheck the patchset.
>> I think codes are ready for committer but should we modify the
>> documentation?
>> {min,max,mean,stddev}_time is now obsoleted so it is better to modify it to
>> {min,max,mean,stddev}_exec_time and add
Hello
> I have reworked that part, adding more comments about the use of GUC
> parameters when establishing the connection to the primary for a WAL
> receiver. And also I have added an extra comment to walreceiver.c
> about the use of GUcs in general, to avoid this stuff again in the
> future.
Hello
>> Well, it seems better to move this patch to next commitfest?
>
> What? You want to make wal_receiver_create_temp_slot unchangeable and
> default to off in pg13, and delay the patch that fixes those things to
> pg14? That makes no sense to me.
I want to handle similar things in a
Hello
Sorry for late replies.
> Yes. In my opinion, patch 0002 should not change the GUC mode of
> wal_receiver_create_temp_slot as the discussion here is about
> primary_conninfo, even if both may share some logic regarding WAL
> receiver shutdown and its restart triggered by the startup
Hello
> I want to start this discussion because this is related to the patch
> (propoesd at the thread [1]) that I'm reviewing. It does that partially,
> i.e., prefers the promotion only when the pause is requested by
> recovery_target_action=pause. But I think that it's reasonable and
> more
Hello
I reviewed a recently published patch. Looks good for me.
One small note: the values for the new definitions in progress.h seems not to
be aligned vertically. However, pgindent doesn't objects.
regards, Sergei
Hello
Thank you for working on this!
> Where this becomes a serious problem is if you have many standbys and you do
> a failover.
+1
Several times my team would like to pause recovery instead of panic after
change settings on primary. (same thing for create_tablespace_directories
replay
Hello
> Thanks for posting this patch, Sergei. Here is a review to make
> things move on.
Thank you, here is updated patch
> The set of comments you are removing from walreceiver.c to decide if a
> temporary slot needs to be created or not should be moved to
> walreceiverfuncs.c as you move the
Hello
Seems bug was introduced in caba97a9d9f4d4fa2531985fd12d3cd823da06f3 - in HEAD
only
In REL_12_STABLE we have:
boolis_recovery_guc_supported = true;
if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_RECOVERY_GUC)
is_recovery_guc_supported =
Hello
Small rebase due to merge conflict of the tests. No functional changes since v7.
PS: also it is end of current CF, I will mark patch entry as moved to the next
CF.diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 2e2af9e96e..7887391bbb 100644
---
Hello
Currently during point-in-time recovery with recovery_target_action = 'pause'
we print log lines:
> LOG: recovery has paused
> HINT: Execute pg_wal_replay_resume() to continue.
My colleague told me that this is a terrible moment: to continue what exactly?
It sounds like "to continue
Hello
> In short, the following things:
> - wal_receiver_create_temp_slot should be made PGC_POSTMASTER,
> similarly to primary_slot_name and primary_conninfo.
> - WalReceiverMain() should not load the parameter from the GUC context
> by itself.
> - RequestXLogStreaming(), called by the startup
Hello
> Yeah, you are right. I was not paying much attention but something
> does not stick here. My understanding is that we should have the WAL
> receiver receive the value it needs to use from the startup process
> (aka via RequestXLogStreaming from xlog.c), and that we ought to make
> this
Hello
"Waiting on Author" is the right status. I found logical conflict with recently
added wal_receiver_create_temp_slot GUC and my tests are currently broken. Will
fix it and post new version.
PS: also, I surprised why it's ok for wal_receiver_create_temp_slot to be
PGC_SIGHUP and ignore
Hello
Thank you!
I am clearly not a good reviewer for such changes... But for a note: I read the
v4 patch and have no useful comments. Good new tests, reasonable code changes
to fix multiple bug reports.
The patch is proposed only for the master branch, right?
regards, Sergei
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation:tested, passed
Thank you!
Looks good to me. I have no further comments. I'll
Hello
> I just thought they were concerned
> that the variable name skip_index might be confusing because we skip
> if skip_index is NOT true.
Right.
>> > - bool skip_index = (get_indstats(lps->lvshared, i) == NULL ||
>> > - skip_parallel_vacuum_index(Irel[i], lps->lvshared));
>> > + bool
Hello
> Yes, we should improve this. I tried to fix this. Attaching a delta
> patch that is fixing both the comments.
Thank you, I have no objections.
I think that status of CF entry is outdated and the most appropriate status for
this patch is "Ready to Commiter". Changed. I also added an
Hello
Thank you, but I am late: patch has another merge conflict.
Conflict seems trivial and patch looks fine for me.
regards, Sergei
Hi
Thank you for update! I looked again
(vacuum_indexes_leader)
+ /* Skip the indexes that can be processed by parallel workers */
+ if (!skip_index)
+ continue;
Does the variable name skip_index not confuse here? Maybe rename to something
like
Hello
I noticed that parallel vacuum uses min_parallel_index_scan_size GUC to skip
small indexes but this is not mentioned in documentation for both vacuum
command and GUC itself.
+ /* Determine the number of parallel workers to launch */
+ if (lps->lvshared->for_cleanup)
+ {
Hello
> As I understand it, lock group is some infrastructure that is used by
> parallel queries, but could be used for something else too. So if
> more documentation is needed, we should say something like "For now,
> only parallel queries can have a lock group" or something like that.
If
Hello
I doubt that "Process ID of the lock group leader" is enough for user
documentation. I think we need note:
- this field is related to parallel query execution
- leader_pid = pid if process is parallel leader
- leader_pid would point to pid of the leader if process is parallel worker
-
Hello
> Attached is a v15 of the online checksums patchset (minus 0005), rebased on
> top
> of your v3 ProcSignalBarrier patch rather than Andres' PoC GlobalBarrier
> patch.
> It does take the, perhaps, controversial approach of replacing the SAMPLE
> barrier with the CHECKSUM barrier. The
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: tested, failed
Spec compliant: not tested
Documentation:tested, passed
Hello
Patch does not apply to master. Could you rebase?
Code looks
Hi
> Andrew Gierth complained about this too over on -committers, and I saw
> his message first and pushed a fix. It includes the first and third
> hunks from your proposed patch, but not the second one.
Yep, I received his email just after sending mine. Thanks, my build is clean
now.
regards,
Hello
> Stellar. If nobody objects in the meantime, I plan to commit 0001-0003
> next week.
My compiler (gcc 8.3.0) is not happy with recent
5910d6c7e311f0b14e3d3cb9ce3597c01d3a3cde commit:
autovacuum.c:831:1: error: ‘AutoVacLauncherShutdown’ was used with no prototype
before its definition
Hello
>> > Also, I'm not entirely sure whether there's anything in our various
>> > replication logic that's dependent on vacuum truncation taking AEL.
>> > Offhand I'd expect the reduced use of AEL to be a plus, but maybe
>> > I'm missing something.
>>
>> It'd be a *MAJOR* plus. One of the
Hi
> I think I got your point. Your proposal is that it's more efficient if
> we make the leader process vacuum the index that can be processed only
> the leader process (i.e. indexes not supporting parallel index vacuum)
> while workers are processing indexes supporting parallel index vacuum,
>
Hi
> I think the advantage of the current approach is that once the parallel
> workers are launched, the leader can process indexes that don't support
> parallelism. So, both type of indexes can be processed at the same time.
In lazy_parallel_vacuum_or_cleanup_indexes I see:
/*
Hello
Its possible to change order of index processing by parallel leader? In v35
patchset I see following order:
- start parallel processes
- leader and parallel workers processed index lixt and possible skip some
entries
- after that parallel leader recheck index list and process the skipped
Hello
> Maybe we need a new elevel category for that.
> SYSTEM_WARNING or LOG_WARNING, perhaps?
I think a separate levels for user warnings and system warnings (and errors)
would be great for log analytics. Error due to user typo in query is not the
same as cache lookup error (for example).
Hello
Could you rebase patch please? I have errors during patch apply. CFbot checks
latest demonstration patch.
> I looked into this. It seems trivial to make walsender create and use a
> temporary replication slot by default if no permanent replication slot
> is specified. This is basically
Hello
> So, I'd like to propose to move the stuff to the second switch().
> (See the attached incomplete patch.) This is rather similar to
> Sergei's previous proposal, but the structure of the state
> machine is kept.
Very similar to my v4 proposal (also move RequestXLogStreaming call), but
Hello
Thank you for review!
> - This parameter can only be set at server start.
> + This parameter can only be set in the postgresql.conf
> + file or on the server command line.
>
> I'm not sure it's good to change the context of this
> description. This was mentioning that changing of this
Hello
Thank you for review!
> ISTM that you need to update the above parts in postgresql.conf.sample.
Good catch, I forgot about conf sample.
> ISTM that you need to update the above comment in walreceiver.c.
Changed
> If primary_conninfo is set to an empty string, walreceiver just shuts
Hello
> Hearing no comments, I've pushed that patch, and marked the v12
> open item closed.
Thank you!
regards, Sergei
Hello
I think the most important question for this topic is performance penalty.
It was a long story, first test on my desktop was too volatile. I setup
separate PC with DB only and test few cases.
PC spec: 2-core Intel Core 2 Duo E6550, 4GB ram, mechanical HDD
All tests on top
Hello
Thank you for attention! I marked CF entry as returned with feedback.
regards, Sergei
1 - 100 of 298 matches
Mail list logo
