Re: Fix comments atop ReorderBufferAddInvalidations
Hi On Thu, Nov 3, 2022 at 7:53 PM Amit Kapila wrote: > > The comments atop seem to indicate that we always accumulate > invalidation messages in top-level transactions which is neither > required nor match with the code. This is introduced in the commit > c55040ccd0 and I have observed it while working on a fix for commit > 16b1fe0037. Thank you for the patch. It looks good to me. I think we can backpatch it to avoid confusion in future. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: Commit fest 2022-11
2022年11月4日(金) 10:23 Ian Lawrence Barwick : > > 2022年11月4日(金) 9:43 Justin Pryzby : > > > > On Thu, Nov 03, 2022 at 11:33:57AM +0900, Ian Lawrence Barwick wrote: > > > 2022年11月2日(水) 19:10 Greg Stark : > > > > > > > > On Tue, 1 Nov 2022 at 06:56, Michael Paquier wrote: > > > > > > > > > Two people showing up to help is really great, thanks! I'll be around > > > > > as well this month, so I'll do my share of patches, as usual. > > > > > > > > Fwiw I can help as well -- starting next week. I can't do much this week though. > > > > > > > > I would suggest starting with the cfbot to mark anything that isn't > > > > applying cleanly and passing tests (and looking for more than design > > > > feedback) as Waiting on Author and reminding the author that it's > > > > commitfest time and a good time to bring the patch into a clean state. > > > > > > Sounds like a plan; I'll make a start on that today/tomorrow as I have > > > some time. > > > > If I'm not wrong, Jacob used the CF app to bulk-mail people about > > patches not applying and similar things. That seemed to work well, and > > doesn't require sending mails to dozens of threads. > > I don't see anything like that in the CF app (though I may be looking in the > wrong place). I also don't see how it would be possible to filter on patches > not applying in cbfot, as AFAICT the former is not aware of the latter.Also, having gone through all the cfbot items with non-applying patches (single red "X"), sending a reminder without checking further doesn't seem the right thing tod do - in two cases the patch was not applying because it had already been committed, and with another the consensus was to return it with feedback. With others, it's obvious the threads were recently active and I don't think a reminder is necessary right now. Please do however let me know if I should be doing something differently. Anyway changes since yesterday: Needs review: 164 -> 156 Waiting on Author: 64 -> 68 Ready for Committer: 22 -> 21 Committed: 43 -> 47 Withdrawn:9 -> 9 Rejected: 1 -> 1 Returned with Feedback: 4 -> 5 Following entries are reported with patch apply failure, but it's less clear (to me) whether a simple request to update the patch is what is needed at this point, because e.g. the thread is long and complex, or has been fairly inactive for a while: - "AcquireExecutorLocks() and run-time pruning" https://commitfest.postgresql.org/40/3478/ - "pg_stat_activity: avoid showing state=active with wait_event=ClientRead" https://commitfest.postgresql.org/40/3760/ - "logical decoding and replication of sequences, take 2" https://commitfest.postgresql.org/40/3823/ - "Lazy JIT IR code generation to increase JIT speed with partitions" https://commitfest.postgresql.org/40/3071/ - "Collation version and dependency helpers" https://commitfest.postgresql.org/40/3977/ - "Time-delayed logical replication subscriber" http://cfbot.cputube.org/patch_40_3581.log - "Fix checkpointer sync request queue problems" https://commitfest.postgresql.org/40/3583/ - "Nonreplayable XLog records by means of overflows and >MaxAllocSize lengths" https://commitfest.postgresql.org/40/3590/ - "Transparent column encryption" https://commitfest.postgresql.org/40/3718/ The following have all received requests to update the patch(s), and have been set to "Waiting on Author" where not already done: - "XID formatting and SLRU refactorings (Independent part of: Add 64-bit XIDs into PostgreSQL 15)" https://commitfest.postgresql.org/40/3489/ (-> patches already updated; changed back to "WfC") - "Add index scan progress to pg_stat_progress_vacuum" https://commitfest.postgresql.org/40/3617/ - "Adding CommandID to heap xlog records" https://commitfest.postgresql.org/40/3882/ - "Add semi-join pushdown to postgres_fdw" https://commitfest.postgresql.org/40/3838/ - "Completed unaccent dictionary with many missing characters" https://commitfest.postgresql.org/40/3631/ - "Data is copied twice when specifying both child and parent table in publication" https://commitfest.postgresql.org/40/3623/ - "Fix ExecRTCheckPerms() inefficiency with many prunable partitions " https://commitfest.postgresql.org/40/3224/ - "In-place persistence change of a relation" https://commitfest.postgresql.org/40/3461/ - "Move SLRU data into the regular buffer pool" https://commitfest.postgresql.org/40/3514/ - "New [relation] options engine" https://commitfest.postgresql.org/40/3536/ - "Nonreplayable XLog records by means of overflows and >MaxAllocSize lengths" https://commitfest.postgresql.org/40/3590/ - "Page compression for OLTP" https://commitfest.postgresql.org/40/3783/ - "Provide the facility to set binary format output for specific OID's per session" https://commitfest.postgresql.org/40/3777/ - "Reducing power consumption when idle" https://commitfest.postgresql.org/40/3566/
Re: Allow single table VACUUM in transaction block
Hi Simon, On Thu, Nov 3, 2022 at 3:53 PM Simon Riggs wrote: > On Tue, 1 Nov 2022 at 23:56, Simon Riggs > wrote: > > > > I haven't checked the rest of the patch, but +1 for allowing VACUUM > FULL > > > within a user txn. > > > > My intention was to prevent that. I am certainly quite uneasy about > > changing anything related to CLUSTER/VF, since they are old, complex > > and bug prone. > > > > So for now, I will block VF, as was my original intent. > > > > I will be guided by what others think... so you may yet get your wish. > > > > > > > Maybe the error message needs to be qualified "...when multiple > > > relations are specified". > > > > > > ERROR: VACUUM cannot run inside a transaction block > > > > Hmm, that is standard wording based on the statement type, but I can > > set a CONTEXT message also. Will update accordingly. > > > > Thanks for your input. > > New version attached, as described. > > Other review comments and alternate opinions welcome. > > I applied and did some basic testing on the patch, it works as described. I would like to bring up a few points that I came across while looking into the vacuum code. 1. As a result of this change to allow VACUUM inside a user transaction, I think there is some chance of causing a block/delay of concurrent VACUUMs if a VACUUM is being run under a long running transaction. 2. Also, if a user runs VACUUM in a transaction, performance optimizations like PROC_IN_VACUUM won't work. 3. Also, if VACUUM happens towards the end of a long running transaction, the snapshot will be old and xmin horizon for vacuum would be somewhat old as compared to current lazy vacuum which acquires a new snapshot just before scanning the table. So, while I understand the need of the feature, I am wondering if there should be some mention of above caveats in documentation with the recommendation that VACUUM should be run outside a transaction, in general. Thank you, Rahila Syed
Re: [PATCH] pgbench: add multiconnect option
2022年4月2日(土) 22:35 Fabien COELHO : > > > > According to the cfbot this patch needs a rebase > > Indeed. v4 attached. Hi cfbot reports the patch no longer applies. As CommitFest 2022-11 is currently underway, this would be an excellent time to update the patch. Thanks Ian Barwick
Re: Privileges on PUBLICATION
On Thu, Nov 3, 2022 at 9:19 PM Peter Eisentraut wrote: > > On 03.11.22 01:43, Antonin Houska wrote: > > Peter Eisentraut wrote: > > > >> The CF entry is about privileges on publications. Please rebase that patch > >> and repost it so that the CF app and the CF bot are up to date. > > > > The rebased patch (with regression tests added) is attached here. > > Some preliminary discussion: > > What is the upgrade strategy? I suppose the options are either that > publications have a default acl that makes them publicly accessible, > thus preserving the existing behavior by default, or pg_dump would need > to create additional GRANT statements when upgrading from pre-PG16. > I think making them publicly accessible is a better option. -- With Regards, Amit Kapila.
Re: Privileges on PUBLICATION
On Thu, Nov 3, 2022 at 11:12 AM Antonin Houska wrote: > > Peter Eisentraut wrote: > > > The CF entry is about privileges on publications. Please rebase that patch > > and repost it so that the CF app and the CF bot are up to date. > > The rebased patch (with regression tests added) is attached here. > > There's still one design issue that I haven't mentioned yet: if the USAGE > privilege on a publication is revoked after the synchronization phase > completed, the missing privilege on a publication causes ERROR in the output > plugin. If the privilege is then granted, the error does not disappear because > the same (historical) snapshot we use to decode the failed data change again > is also used to check the privileges in the catalog, so the output plugin does > not see that the privilege has already been granted. > We have a similar problem even when publication is dropped/created. The replication won't be able to proceed. > The only solution seems to be to drop the publication from the subscription > and add it again, or to drop and re-create the whole subscription. I haven't > added a note about this problem to the documentation yet, in case someone has > better idea how to approach the problem. > I think one possibility is that the user advances the slot used in replication by using pg_replication_slot_advance() at or after the location where the privilege is granted. Some other ideas have been discussed in the thread [1], in particular, see email [2] and discussion after that but we didn't reach any conclusion. [1] - https://www.postgresql.org/message-id/CAHut%2BPvMbCsL8PAz1Qc6LNoL0Ag0y3YJtPVJ8V0xVXJOPb%2B0xw%40mail.gmail.com [2] - https://www.postgresql.org/message-id/CAA4eK1JTwOAniPua04o2EcOXfzRa8ANax%3D3bpx4H-8dH7M2p%3DA%40mail.gmail.com -- With Regards, Amit Kapila.
Re: archive modules
2022年10月16日(日) 16:36 Bharath Rupireddy : > > On Sat, Oct 15, 2022 at 3:13 AM Nathan Bossart > wrote: > > > > On Fri, Oct 14, 2022 at 11:51:30AM -0700, Nathan Bossart wrote: > > > On Fri, Oct 14, 2022 at 12:10:18PM +0530, Bharath Rupireddy wrote: > > >> 2) I think we have a problem - set archive_mode and archive_library > > >> and start the server, then set archive_command, reload the conf, see > > >> [3] - the archiver needs to error out right? The archiver gets > > >> restarted whenever archive_library changes but not when > > >> archive_command changes. I think the right place for the error is > > >> after or at the end of HandlePgArchInterrupts(). > > > > > > Good catch. You are right, this is broken. I believe that we need to > > > check for the misconfiguration in HandlePgArchInterrupts() in addition to > > > LoadArchiveLibrary(). I will work on fixing this. > > > > As promised... > > Thanks. I think that if the condition can be simplified something like > in the attached. It's okay to call shutdown callback twice by getting > rid of the comment [1] as it doesn't add any extra value or > information, it just says that we're calling shutdown callback > function. With the attached, the code is more readable and the > footprint of the changes are reduced. > > [1] > /* > * Call the currently loaded archive module's shutdown callback, > * if one is defined. > */ > call_archive_module_shutdown_callback(0, 0); Hi cfbot reports the patch no longer applies [1]. As CommitFest 2022-11 is currently underway, this would be an excellent time to update the patch. [1] http://cfbot.cputube.org/patch_40_3933.log Thanks Ian Barwick
Re: Condition pushdown: why (=) is pushed down into join, but BETWEEN or >= is not?
2022年7月7日(木) 20:11 Andrey Lepikhov : > > On 17/5/2022 05:00, Andy Fan wrote: > > Thanks. But I will wait to see if anyone will show interest with this. > > Or else > > Moving alone is not a great experience. > To move forward I've rebased your patchset onto new master, removed > annoying tailing backspaces and applied two regression test changes, > caused by second patch: first of changes are legal, second looks normal > but should be checked on optimality. > As I see, a consensus should be found for the questions: > 1. Case of redundant clauses (x < 100 and x < 1000) > 2. Planning time degradation for trivial OLTP queries Hi cfbot reports the patch no longer applies [1]. As CommitFest 2022-11 is currently underway, this would be an excellent time to update the patch. [1] http://cfbot.cputube.org/patch_40_3524.log Thanks Ian Barwick
Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL
2022年5月3日(火) 8:45 David Christensen : > > ...and pushing a couple fixups pointed out by cfbot, so here's v4. Hi cfbot reports the patch no longer applies [1]. As CommitFest 2022-11 is currently underway, this would be an excellent time to update the patch. [1] http://cfbot.cputube.org/patch_40_3628.log Thanks Ian Barwick
Re: Skipping schema changes in publication
2022年8月19日(金) 2:41 vignesh C :
>
> On Mon, Aug 8, 2022 at 2:53 PM vignesh C wrote:
> >
> > On Mon, Aug 8, 2022 at 12:46 PM vignesh C wrote:
> > >
> > > On Fri, Jun 3, 2022 at 3:36 PM vignesh C wrote:
> > > >
> > > > On Thu, May 26, 2022 at 7:04 PM osumi.takami...@fujitsu.com
> > > > wrote:
> > > > >
> > > > > On Monday, May 23, 2022 2:13 PM vignesh C wrote:
> > > > > > Attached v7 patch which fixes the buildfarm warning for an unused
> > > > > > warning in
> > > > > > release mode as in [1].
> > > > > Hi, thank you for the patches.
> > > > >
> > > > >
> > > > > I'll share several review comments.
> > > > >
> > > > > For v7-0001.
> > > > >
> > > > > (1) I'll suggest some minor rewording.
> > > > >
> > > > > +
> > > > > + The RESET clause will reset the publication to
> > > > > the
> > > > > + default state which includes resetting the publication options,
> > > > > setting
> > > > > + ALL TABLES flag to false and
> > > > > + dropping all relations and schemas that are associated with the
> > > > > publication.
> > > > >
> > > > > My suggestion is
> > > > > "The RESET clause will reset the publication to the
> > > > > default state. It resets the publication operations,
> > > > > sets ALL TABLES flag to false and drops all relations
> > > > > and schemas associated with the publication."
> > > >
> > > > I felt the existing looks better. I would prefer to keep it that way.
> > > >
> > > > > (2) typo and rewording
> > > > >
> > > > > +/*
> > > > > + * Reset the publication.
> > > > > + *
> > > > > + * Reset the publication options, setting ALL TABLES flag to false
> > > > > and drop
> > > > > + * all relations and schemas that are associated with the
> > > > > publication.
> > > > > + */
> > > > >
> > > > > The "setting" in this sentence should be "set".
> > > > >
> > > > > How about changing like below ?
> > > > > FROM:
> > > > > "Reset the publication options, setting ALL TABLES flag to false and
> > > > > drop
> > > > > all relations and schemas that are associated with the publication."
> > > > > TO:
> > > > > "Reset the publication operations, set ALL TABLES flag to false and
> > > > > drop
> > > > > all relations and schemas associated with the publication."
> > > >
> > > > I felt the existing looks better. I would prefer to keep it that way.
> > > >
> > > > > (3) AlterPublicationReset
> > > > >
> > > > > Do we need to call CacheInvalidateRelcacheAll() or
> > > > > InvalidatePublicationRels() at the end of
> > > > > AlterPublicationReset() like AlterPublicationOptions() ?
> > > >
> > > > CacheInvalidateRelcacheAll should be called if we change all tables
> > > > from true to false, else the cache will not be invalidated. Modified
> > > >
> > > > >
> > > > > For v7-0002.
> > > > >
> > > > > (4)
> > > > >
> > > > > + if (stmt->for_all_tables)
> > > > > + {
> > > > > + boolisdefault =
> > > > > CheckPublicationDefValues(tup);
> > > > > +
> > > > > + if (!isdefault)
> > > > > + ereport(ERROR,
> > > > > +
> > > > > errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> > > > > + errmsg("adding ALL TABLES
> > > > > requires the publication to have default publication options, no
> > > > > tables/
> > > > > + errhint("Use ALTER
> > > > > PUBLICATION ... RESET to reset the publication"));
> > > > >
> > > > >
> > > > > The errmsg string has three messages for user and is a bit long
> > > > > (we have two sentences there connected by 'and').
> > > > > Can't we make it concise and split it into a couple of lines for code
> > > > > readability ?
> > > > >
> > > > > I'll suggest a change below.
> > > > > FROM:
> > > > > "adding ALL TABLES requires the publication to have default
> > > > > publication options, no tables/schemas associated and ALL TABLES flag
> > > > > should not be set"
> > > > > TO:
> > > > > "adding ALL TABLES requires the publication defined not for ALL
> > > > > TABLES"
> > > > > "to have default publish actions without any associated
> > > > > tables/schemas"
> > > >
> > > > Added errdetail and split it
> > > >
> > > > > (5) typo
> > > > >
> > > > >
> > > > > +EXCEPT TABLE
> > > > > +
> > > > > +
> > > > > + This clause specifies a list of tables to exclude from the
> > > > > publication.
> > > > > + It can only be used with FOR ALL TABLES.
> > > > > +
> > > > > +
> > > > > +
> > > > > +
> > > > >
> > > > > Kindly change
> > > > > FROM:
> > > > > This clause specifies a list of tables to exclude from the
> > > > > publication.
> > > > > TO:
> > > > > This clause specifies a list of tables to be excluded from the
> > > > > publication.
> > > > > or
> > > > > This clause specifies a list of tables excluded from the publication.
> > > >
> > > > Modified
> > > >
> > > > > (6) Minor suggestion for an expression change
> > > > >
> > > > >
Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication
2022年8月5日(金) 22:55 Melih Mutlu : > > Hi Amit, > >> >> Why after step 4, do you need to drop the replication slot? Won't just >> >> clearing the required info from the catalog be sufficient? >> > >> > >> > The replication slots that we read from the catalog will not be used for >> > anything else after we're done with syncing the table which the rep slot >> > belongs to. >> > It's removed from the catalog when the sync is completed and it basically >> > becomes a slot that is not linked to any table or worker. That's why I >> > think it should be dropped rather than left behind. >> > >> > Note that if a worker dies and its replication slot continues to exist, >> > that slot will only be used to complete the sync process of the one table >> > that the dead worker was syncing but couldn't finish. >> > When that particular table is synced and becomes ready, the replication >> > slot has no use anymore. >> > >> >> Why can't it be used to sync the other tables if any? > > > It can be used. But I thought it would be better not to, for example in the > following case: > Let's say a sync worker starts with a table in INIT state. The worker creates > a new replication slot to sync that table. > When sync of the table is completed, it will move to the next one. This time > the new table may be in FINISHEDCOPY state, so the worker may need to use the > new table's existing replication slot. > Before the worker will move to the next table again, there will be two > replication slots used by the worker. We might want to keep one and drop the > other. > At this point, I thought it would be better to keep the replication slot > created by this worker in the first place. I think it's easier to track slots > this way since we know how to generate the rep slots name. > Otherwise we would need to store the replication slot name somewhere too. > > >> >> This sounds reasonable. Let's do this unless we get some better idea. > > > I updated the patch to use an unique id for replication slot names and store > the last used id in the catalog. > Can you look into it again please? > > >> There is no such restriction that origins should belong to only one >> table. What makes you think like that? > > > I did not reuse origins since I didn't think it would significantly improve > the performance as reusing replication slots does. > So I just kept the origins as they were, even if it was possible to reuse > them. Does that make sense? Hi cfbot reports the patch no longer applies [1]. As CommitFest 2022-11 is currently underway, this would be an excellent time to update the patch. [1] http://cfbot.cputube.org/patch_40_3784.log Thanks Ian Barwick
Re: Reducing power consumption on idle servers
2022年3月25日(金) 1:03 Simon Riggs : > > On Thu, 24 Mar 2022 at 15:39, Robert Haas wrote: > > > > On Thu, Mar 24, 2022 at 6:59 AM Simon Riggs > > wrote: > > > The proposals of this patch are the following, each of which can be > > > independently accepted/rejected: > > > 1. fix the sleep pattern of bgwriter, walwriter and logical worker > > > (directly affects powersave) > > > 2. deprecate promote_trigger_file, which then allows us to fix the > > > sleep for startup process (directly affects powersave) > > > 3. treat hibernation in all procs the same, for simplicity, and to > > > make sure we don't forget one later > > > 4. provide a design pattern for background worker extensions to > > > follow, so as to encourage powersaving > > > > Unfortunately, the patch isn't split in a way that corresponds to this > > division. I think I'm +1 on change #2 -- deprecating > > promote_trigger_file seems like a good idea to me independently of any > > power-saving considerations. But I'm not sure that I am on board with > > any of the other changes. > > OK, so change (2) is good. Separate patch attached. Hi cfbot reports the patch no longer applies [1]. As CommitFest 2022-11 is currently underway, this would be an excellent time to update the patch. [1] http://cfbot.cputube.org/patch_40_3566.log Thnks Ian Barwick
Re: Proposal to provide the facility to set binary format output for specific OID's per session
2022年9月6日(火) 21:32 Dave Cramer : > > > > > On Tue, 6 Sept 2022 at 02:30, Ibrar Ahmed wrote: >> >> >> >> On Fri, Aug 12, 2022 at 5:48 PM Dave Cramer wrote: >>> >>> >>> >>> On Fri, 5 Aug 2022 at 17:51, Justin Pryzby wrote: On Tue, Jul 26, 2022 at 08:11:04AM -0400, Dave Cramer wrote: > Attached patch to correct these deficiencies. You sent a patch to be applied on top of the first patch, but cfbot doesn't know that, so it says the patch doesn't apply. http://cfbot.cputube.org/dave-cramer.html BTW, a previous discussion about this idea is here: https://www.postgresql.org/message-id/flat/40cbb35d-774f-23ed-3079-03f938aac...@2ndquadrant.com >>> >>> >>> squashed patch attached >>> >>> Dave >> >> The patch does not apply successfully; a rebase is required. >> >> === applying patch ./0001-add-format_binary.patch >> patching file src/backend/tcop/postgres.c >> Hunk #1 succeeded at 97 (offset -8 lines). >> patching file src/backend/tcop/pquery.c >> patching file src/backend/utils/init/globals.c >> patching file src/backend/utils/misc/guc.c >> Hunk #1 succeeded at 144 (offset 1 line). >> Hunk #2 succeeded at 244 with fuzz 2 (offset 1 line). >> Hunk #3 succeeded at 4298 (offset -1 lines). >> Hunk #4 FAILED at 12906. >> 1 out of 4 hunks FAILED -- saving rejects to file >> src/backend/utils/misc/guc.c.rej >> patching file src/include/miscadmin.h >> > > Thanks, > > New rebased patch attached Hi cfbot reports the patch no longer applies [1]. As CommitFest 2022-11 is currently underway, this would be an excellent time to update the patch again. [1] http://cfbot.cputube.org/patch_40_3777.log Thanks Ian Barwick
Re: [Proposal] Page Compression for OLTP
2022年7月27日(水) 2:47 chenhj : > > Hi hackers, > > I have rebase this patch and made some improvements. > > > 1. A header is added to each chunk in the pcd file, which records the chunk > of which block the chunk belongs to, and the checksum of the chunk. > > Accordingly, all pages in a compressed relation are stored in compressed > format, even if the compressed page is larger than BLCKSZ. > > The maximum space occupied by a compressed page is BLCKSZ + chunk_size > (exceeding this range will report an error when writing the page). > > 2. Repair the pca file through the information recorded in the pcd when > recovering from a crash > > 3. For compressed relation, do not release the free blocks at the end of the > relation (just like what old_snapshot_threshold does), reducing the risk of > data inconsistency between pcd and pca file. > > 4. During backup, only check the checksum in the chunk header for the pcd > file, and avoid assembling and decompressing chunks into the original page. > > 5. bugfix, doc, code style and so on > > > And see src/backend/storage/smgr/README.compression for detail > > > Other > > 1. remove support of default compression option in tablespace, I'm not sure > about the necessity of this feature, so don't support it for now. > > 2. pg_rewind currently does not support copying only changed blocks from pcd > file. This feature is relatively independent and could be implemented later. Hi cfbot reports the patch no longer applies. As CommitFest 2022-11 is currently underway, this would be an excellent time to update the patch. Thanks Ian Barwick
Re: Commit fest 2022-11
2022年11月4日(金) 9:43 Justin Pryzby : > > On Thu, Nov 03, 2022 at 11:33:57AM +0900, Ian Lawrence Barwick wrote: > > 2022年11月2日(水) 19:10 Greg Stark : > > > > > > On Tue, 1 Nov 2022 at 06:56, Michael Paquier wrote: > > > > > > > Two people showing up to help is really great, thanks! I'll be around > > > > as well this month, so I'll do my share of patches, as usual. > > > > > > Fwiw I can help as well -- starting next week. I can't do much this week > > > though. > > > > > > I would suggest starting with the cfbot to mark anything that isn't > > > applying cleanly and passing tests (and looking for more than design > > > feedback) as Waiting on Author and reminding the author that it's > > > commitfest time and a good time to bring the patch into a clean state. > > > > Sounds like a plan; I'll make a start on that today/tomorrow as I have > > some time. > > If I'm not wrong, Jacob used the CF app to bulk-mail people about > patches not applying and similar things. That seemed to work well, and > doesn't require sending mails to dozens of threads. I don't see anything like that in the CF app (though I may be looking in the wrong place). I also don't see how it would be possible to filter on patches not applying in cbfot, as AFAICT the former is not aware of the latter. There is an option for each entry to send an email from the CF app, but it comes with a note "Please ensure that the email settings for your domain (DKIM, SPF) allow emails from external sources." which I fear would lead to email delivery issues. Regards Ian Barwick
Re: On login trigger: take three
2022年11月4日(金) 5:23 Daniel Gustafsson : > > > On 20 Sep 2022, at 16:10, Daniel Gustafsson wrote: > > > Since the original authors seems to have retired from the patch > > (I've only rebased it to try and help) I am inclined to mark it as returned > > with feedback. > > Doing so now since no updates have been posted for quite some time and holes > have been poked in the patch. I see it was still "Waiting on Author" so I went ahead and changed the status. > The GUC for temporarily disabling event triggers, to avoid the need for > single- > user mode during troubleshooting, might however be of interest so I will break > that out and post to a new thread. For reference this is the new thread: https://www.postgresql.org/message-id/9140106E-F9BF-4D85-8FC8-F2D3C094A6D9%40yesql.se Regards Ian Barwick
Re: Incorrect include file order in guc-file.l
On Fri, Nov 4, 2022 at 5:42 AM Michael Paquier wrote: > > The CI is able to complete without it. Would you mind if it is > removed? If you don't want us to poke more at the bear, that's a nit > so leaving things as they are is also fine by me. I've removed it. -- John Naylor EDB: http://www.enterprisedb.com
Re: Non-replayable WAL records through overflows and >MaxAllocSize lengths
2022年10月5日(水) 16:46 Michael Paquier : > > Hi Matthias, > > On Wed, Jul 27, 2022 at 02:07:05PM +0200, Matthias van de Meent wrote: > > My apologies for the time it took me to come back to this thread. > > > + * To accommodate some overhead, hhis MaxXLogRecordSize value allows for > > > s/hhis/this/. > > > > Will be included in the next update.. > > v8 fails to apply. Could you send a rebased version? > > As far as I recall the problems with the block image sizes are solved, > but we still have a bit more to do in terms of the overall record > size. Perhaps there are some parts of the patch you'd like to > revisit? > > For now, I have switched the back as waiting on author, and moved it > to the next CF. Hi Matthias CommitFest 2022-11 is currently underway, so if you are interested in moving this patch forward, now would be a good time to update it. Thanks Ian Barwick
Re: [PATCH] New [relation] option engine
2022年7月12日(火) 13:47 Nikolay Shaplov : > > В письме от вторник, 12 июля 2022 г. 07:30:40 MSK пользователь Nikolay Shaplov > написал: > > > What about table access methods? There have been a couple attempts to > > > allow custom reloptions for table AMs. Does this patch help that use > > > case? > > > > This patch does not add custom reloptions for table AM. It is already huge > > enough, with no extra functionality. But new option engine will make adding > > custom options for table AM more easy task, as main goal of this patch is to > > simplify adding options everywhere they needed. And yes, adding custom > > table AM options is one of my next goals, as soon as this patch is commit. > > And here goes rebased version of the patch, that can be applied to current > master. Hi cfbot reports the patch no longer applies. As CommitFest 2022-11 is currently underway, this would be an excellent time to update the patch. Thanks Ian Barwick
Re: SLRUs in the main buffer pool - Page Header definitions
2022年9月28日(水) 10:57 Bagga, Rishu : > > Hi, > > We have been working on adding page headers to the SLRU pages, as part of the > migration for SLRU to buffer cache. We’ve incorporated Thomas Munro’s patch > and Heikki’s Storage manager changes[1] and have a patch for early feedback. > > As part of our changes we have: > > 1. Added page headers to the following > >*Commit_TS > *CLOG > *MultiXact > *Subtrans > *Serial (predicate.c) > *Notify (async.c) > > For commit_ts, clog and MultiXact, the PageXLogRecPtr field is populated with > the LSN returned during the creation of a new page; as there is no WAL record > for the rest, PageXLogRecPtr is set to “InvalidXlogRecPtr”. > > There is one failing assert in predicate.c for SerialPagePrecedes with the > page header changes; we are looking into this issue. > > The page_version is set to PG_METAPAGE_LAYOUT_VERSION (which is 1) > > > 2. Change block number passed into ReadSlruBuffer from relative to absolute, > and account for SLRU’s 256kb segment size in md.c. > > > > The changes pass the regression tests. We are still working on handling the > upgrade scenario and should have a patch out for that soon. > > Attached is the patch with all changes (Heikki and Munro’s patch and page > headers) consolidated Hi cfbot reports the patch no longer applies. As CommitFest 2022-11 is currently underway, this would be an excellent time to update the patch. Thanks Ian Barwick
Re: Commit fest 2022-11
On Thu, Nov 03, 2022 at 11:33:57AM +0900, Ian Lawrence Barwick wrote: > 2022年11月2日(水) 19:10 Greg Stark : > > > > On Tue, 1 Nov 2022 at 06:56, Michael Paquier wrote: > > > > > Two people showing up to help is really great, thanks! I'll be around > > > as well this month, so I'll do my share of patches, as usual. > > > > Fwiw I can help as well -- starting next week. I can't do much this week > > though. > > > > I would suggest starting with the cfbot to mark anything that isn't > > applying cleanly and passing tests (and looking for more than design > > feedback) as Waiting on Author and reminding the author that it's > > commitfest time and a good time to bring the patch into a clean state. > > Sounds like a plan; I'll make a start on that today/tomorrow as I have > some time. If I'm not wrong, Jacob used the CF app to bulk-mail people about patches not applying and similar things. That seemed to work well, and doesn't require sending mails to dozens of threads. -- Justin
Re: In-placre persistance change of a relation
2022年9月28日(水) 17:21 Kyotaro Horiguchi : > > Just rebased. Hi cfbot reports the patch no longer applies. As CommitFest 2022-11 is currently underway, this would be an excellent time to update the patch. Thanks Ian Barwick
Re: security_context_t marked as deprecated in libselinux 3.1
On Thu, Nov 03, 2022 at 07:01:20PM -0400, Tom Lane wrote: > Alvaro Herrera writes: >> FWIW I just had a CI job fail the "warnings" test because of lacking >> this patch in the back branches :-) What do you think about >> back-patching this to at least 11? > > No objection to back-patching from me. Fine by me. >> I would say 10, but since that one >> is going to end soon, it might not be worth much effort. OTOH maybe we >> want to backpatch all the way back to 9.2 given the no-warnings policy >> we recently acquired. > > I'm not sure that no-warnings policy extends to stuff as far off the > beaten path as sepgsql. However, I won't stand in the way if you > want to do that. One point though: if you want to touch v10, I'd > suggest waiting till after next week's releases. Unlikely as it > is that this'd break anything, I don't think we should risk it > in the branch's last release. In line of ad96696, seems like that it would make sense to do the same here even if the bar is lower. sepgsql has not changed in years, so I suspect few conflicts. Alvaro, if you want to take care of that, that's fine by me. I could do it, but not before next week. Agreed to wait after the next minor release. -- Michael signature.asc Description: PGP signature
Re: ExecRTCheckPerms() and many prunable partitions
2022年10月15日(土) 15:01 Amit Langote : > > On Fri, Oct 7, 2022 at 4:31 PM Amit Langote wrote: > > On Fri, Oct 7, 2022 at 3:49 PM Amit Langote wrote: > > > On Fri, Oct 7, 2022 at 1:25 PM Amit Langote > > > wrote: > > > > On Fri, Oct 7, 2022 at 10:04 AM Amit Langote > > > > wrote: > > > > > On Thu, Oct 6, 2022 at 10:29 PM Amit Langote > > > > > wrote: > > > > > > Actually, List of Bitmapsets turned out to be something that doesn't > > > > > > just-work with our Node infrastructure, which I found out thanks to > > > > > > -DWRITE_READ_PARSE_PLAN_TREES. So, I had to go ahead and add > > > > > > first-class support for copy/equal/write/read support for > > > > > > Bitmapsets, > > > > > > such that writeNode() can write appropriately labeled versions of > > > > > > them > > > > > > and nodeRead() can read them as Bitmapsets. That's done in 0003. I > > > > > > didn't actually go ahead and make *all* Bitmapsets in the plan trees > > > > > > to be Nodes, but maybe 0003 can be expanded to do that. We won't > > > > > > need > > > > > > to make gen_node_support.pl emit *_BITMAPSET_FIELD() blurbs then; > > > > > > can > > > > > > just use *_NODE_FIELD(). > > > > > > > > > > All meson builds on the cfbot machines seem to have failed, maybe > > > > > because I didn't update src/include/nodes/meson.build to add > > > > > 'nodes/bitmapset.h' to the `node_support_input_i` collection. Here's > > > > > an updated version assuming that's the problem. (Will set up meson > > > > > builds on my machine to avoid this in the future.) > > > > > > > > And... noticed that a postgres_fdw test failed, because > > > > _readBitmapset() not having been changed to set NodeTag would > > > > "corrupt" any Bitmapsets that were created with it set. > > > > > > Broke the other cases while fixing the above. Attaching a new version > > > again. In the latest version, I'm setting Bitmapset.type by hand with > > > an XXX comment nearby saying that it would be nice to change that to > > > makeNode(Bitmapset), which I know sounds pretty ad-hoc. > > > > Sorry, I attached the wrong patches with the last email. The > > "correct" v22 attached this time. > > Rebased over c037471832. This entry was marked as "Needs review" in the CommitFest app but cfbot reports the patch no longer applies. We've marked it as "Waiting on Author". As CommitFest 2022-11 is currently underway, this would be an excellent time update the patch. Once you think the patchset is ready for review again, you (or any interested party) can move the patch entry forward by visiting https://commitfest.postgresql.org/40/3224/ and changing the status to "Needs review". Thanks Ian Barwick
Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert
On Tue, Nov 1, 2022 at 10:55 AM Jacob Champion wrote: > On Tue, Nov 1, 2022 at 10:03 AM Jacob Champion > wrote: > > I'm not familiar with "unregistered scheme" in this context and will > > need to dig in. > > Unfortunately I can't reproduce with 3.0.0 on Ubuntu :( Sorry, when rereading my own emails I suspect they didn't make much sense to readers. The failure I'm talking about is in cfbot [1], on the Monterey/Meson build, which is using OpenSSL 3.0.0. I unfortunately cannot reproduce this on my own Ubuntu machine. There is an additional test failure with LibreSSL, which doesn't appear to honor the SSL_CERT_FILE environment variable. This isn't a problem in production -- if you're using LibreSSL, you'd presumably understand that you can't use that envvar -- but it makes testing difficult, because I don't yet know a way to tell LibreSSL to use a different set of roots for the duration of a test. Has anyone dealt with this before? > If there are no valuable use cases for weaker checks, then we could go > even further than my 0002 and just reject any weaker sslmodes > outright. That'd be nice. I plan to take this approach in a future v3, with the opinion that it'd be better for this feature to start life as strict as possible. --Jacob [1] https://cirrus-ci.com/task/6176610722775040
Re: [PATCH] Expand character set for ltree labels
2022年10月6日(木) 7:05 Garen Torikian : > > After digging into it, you are completely correct. I had to do a bit more > reading to understand the relationships between UTF-8 and wchar, but > ultimately the existing locale support works for my use case. > > Therefore I have updated the patch with three much smaller changes: > > * Support for `-` in addition to `_` > * Expanding the limit to 512 chars (from the existing 256); again it's not > uncommon for non-English strings to be much longer > * Fixed the documentation to expand on what the ltree label's relationship to > the DB locale is > > Thank you, > Garen This entry was marked as "Needs review" in the CommitFest app but cfbot reports the patch no longer applies. We've marked it as "Waiting on Author". As CommitFest 2022-11 is currently underway, this would be an excellent time update the patch. Once you think the patchset is ready for review again, you (or any interested party) can move the patch entry forward by visiting https://commitfest.postgresql.org/40/3929/ and changing the status to "Needs review". Thanks Ian Barwick
Re: Data is copied twice when specifying both child and parent table in publication
2022年10月17日(月) 14:49 wangw.f...@fujitsu.com :
>
> On Wed, Oct 5, 2022 at 11:08 AM Peter Smith wrote:
> > Hi Wang-san. Here are my review comments for HEAD_v12-0001 patch.
>
> Thanks for your comments.
>
> > ==
> >
> > 1. Missing documentation.
> >
> > In [1] you wrote:
> > > I think the behaviour of multiple publications with parameter
> > publish_via_partition_root could be added to the pg-doc later in a separate
> > patch.
> >
> > ~
> >
> > That doesn't seem right to me. IMO the related documentation updates
> > cannot really be separated from this patch. Otherwise, what's the
> > alternative? Push this change, and then (while waiting for the
> > documentation patch) users will just have to use trial and error to
> > guess how it works...?
>
> I tried to add related documentation in a separate patch (HEAD_v13-0002*).
>
> > --
> >
> > 2. src/backend/catalog/pg_publication.c
> >
> > + typedef struct
> > + {
> > + Oid relid; /* OID of published table */
> > + Oid pubid; /* OID of publication that publishes this
> > + * table. */
> > + } published_rel;
> >
> > 2a.
> > I think that should be added to typedefs.list
>
> Added.
>
> > ~
> >
> > 2b.
> > Maybe this also needs some comment to clarify that there will be
> > *multiple* of these structures in scenarios where the same table is
> > published by different publications in the array passed.
>
> Added the comments.
>
> > --
> >
> > 3. QUESTION - pg_get_publication_tables / fetch_table_list
> >
> > When the same table is published by different publications (but there
> > are other differences like row-filters/column-lists in each
> > publication) the result tuple of this function does not include the
> > pubid. Maybe the SQL of pg_publication_tables/fetch_table_list() is OK
> > as-is but how does it manage to associate each table with the correct
> > tuple?
> >
> > I know it apparently all seems to work but I’m not how does that
> > happen? Can you explain why a puboid is not needed for the result
> > tuple of this function?
>
> Sorry, I am not sure I understand your question.
> I try to answer your question by explaining the two functions you mentioned:
>
> First, the function pg_get_publication_tables gets the list (see table_infos)
> that included published table and the corresponding publication. Then based
> on this list, the function pg_get_publication_tables returns information
> (scheme, relname, row filter and column list) about the published tables in
> the
> publications list. It just doesn't return pubid.
>
> Then, the SQL in the function fetch_table_list will get the columns in the
> column list from pg_attribute. (This is to return all columns when the column
> list is not specified)
>
> > ~~
> >
> > test_pub=# create table t1(a int, b int, c int);
> > CREATE TABLE
> > test_pub=# create publication pub1 for table t1(a) where (a > 99);
> > CREATE PUBLICATION
> > test_pub=# create publication pub2 for table t1(a,b) where (b < 33);
> > CREATE PUBLICATION
> >
> > Following seems OK when I swap orders of publication names...
> >
> > test_pub=# SELECT gpt.relid, gpt.attrs, pg_get_expr(gpt.qual,
> > gpt.relid) AS rowfilter from pg_get_publication_tables(VARIADIC
> > ARRAY['pub2','pub1']) gpt(relid, attrs, qual);
> > relid | attrs | rowfilter
> > ---+---+---
> > 16385 | 1 2 | (b < 33)
> > 16385 | 1 | (a > 99)
> > (2 rows)
> >
> > test_pub=# SELECT gpt.relid, gpt.attrs, pg_get_expr(gpt.qual,
> > gpt.relid) AS rowfilter from pg_get_publication_tables(VARIADIC
> > ARRAY['pub1','pub2']) gpt(relid, attrs, qual);
> > relid | attrs | rowfilter
> > ---+---+---
> > 16385 | 1 | (a > 99)
> > 16385 | 1 2 | (b < 33)
> > (2 rows)
> >
> > But what about this (this is similar to the SQL fragment from
> > fetch_table_list); I swapped the pub names but the results are the
> > same...
> >
> > test_pub=# SELECT pg_get_publication_tables(VARIADIC
> > array_agg(p.pubname)) from pg_publication p where pubname
> > IN('pub2','pub1');
> >
> > pg_get_publication_tables
> >
> > -
> > -
> > -
> > -
> > ---
> > (16385,1,"{OPEXPR :opno 521 :opfuncid 147 :opresulttype 16 :opretset
> > false :opcollid 0 :inputcollid 0 :args ({VAR :varno 1 :varattno 1
> > :vartype 23 :vartypmod -1 :var
> > collid 0 :varlevelsup 0 :varnosyn 1 :varattnosyn 1 :location 47}
> > {CONST :consttype 23 :consttypmod -1 :constcollid 0 :constlen 4
> > :constbyval true :constisnull false :
> > location 51 :constvalue 4 [ 99 0 0 0 0 0 0 0 ]}) :location 49}")
> > (16385,"1 2","{OPEXPR :opno 97 :opfuncid 66 :opresulttype 16
> > :opretset false
Re: [PATCH] Completed unaccent dictionary with many missing characters
2022年7月13日(水) 19:13 Przemysław Sztoch : > > Dear Michael P., > > 3. The matter is not that simple. When I change priorities (ie > Latin-ASCII.xml is less important than Unicode decomposition), > then "U + 33D7" changes not to pH but to PH. > In the end, I left it like it was before ... > > If you decide what to do with point 3, I will correct it and send new patches. > > What is your decision? > Option 1: We leave x as in Latin-ASCII.xml and we also have full > compatibility with previous PostgreSQL versions. > If they fix Latin-ASCII.xml at Unicode, it will fix itself. > > Option 2: We choose a lower priority for entries in Latin-ASCII.xml > > I would choose option 1. > > P.S. I will be going on vacation and it would be nice to close this patch > soon. TIA. Hi This entry was marked as "Needs review" in the CommitFest app but cfbot reports the patch no longer applies. We've marked it as "Waiting on Author". As CommitFest 2022-11 is currently underway, this would be an excellent time update the patch. Once you think the patchset is ready for review again, you (or any interested party) can move the patch entry forward by visiting https://commitfest.postgresql.org/40/3631/ and changing the status to "Needs review". Thanks Ian Barwick
Re: Add semi-join pushdown to postgres_fdw
2022年8月30日(火) 15:58 Alexander Pyhalov :
>
> Ashutosh Bapat писал 2022-08-29 17:12:
> > Hi Alexander,
> > Thanks for working on this. It's great to see FDW join pushdown scope
> > being expanded to more complex cases.
> >
> > I am still figuring out the implementation. It's been a while I have
> > looked at join push down code.
> >
> > But following change strikes me odd
> > -- subquery using immutable function (can be sent to remote)
> > PREPARE st3(int) AS SELECT * FROM ft1 t1 WHERE t1.c1 < $2 AND t1.c3
> > IN (SELECT c3 FROM ft2 t2 WHERE c1 > $1 AND date(c5) =
> > '1970-01-17'::date) ORDER BY c1;
> > EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st3(10, 20);
> > - QUERY PLAN
> >
> > - Sort
> > +
> >
> > QUERY PLAN
> > +---
> > + Foreign Scan
> > Output: t1.c1, t1.c2, t1.c3, t1.c4, t1.c5, t1.c6, t1.c7, t1.c8
> > - Sort Key: t1.c1
> > - -> Nested Loop Semi Join
> > - Output: t1.c1, t1.c2, t1.c3, t1.c4, t1.c5, t1.c6, t1.c7,
> > t1.c8
> > - Join Filter: (t1.c3 = t2.c3)
> > - -> Foreign Scan on public.ft1 t1
> > - Output: t1.c1, t1.c2, t1.c3, t1.c4, t1.c5, t1.c6,
> > t1.c7, t1.c8
> > - Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8
> > FROM "S 1"."T 1" WHERE (("C 1" < 20))
> > - -> Materialize
> > - Output: t2.c3
> > - -> Foreign Scan on public.ft2 t2
> > - Output: t2.c3
> > - Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE
> > (("C 1" > 10)) AND ((date(c5) = '1970-01-17'::date))
> > -(14 rows)
> > + Relations: (public.ft1 t1) SEMI JOIN (public.ft2 t2)
> > + Remote SQL: SELECT r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6,
> > r1.c7, r1.c8 FROM "S 1"."T 1" r1 WHERE ((r1."C 1" < 20)) AND (EXISTS
> > (SELECT NULL FROM "S 1"."T 1" r3 WHERE ((r3."C 1" > 10)) AND
> > ((date(r3.c5) = '1970-01-17'::date)) AND ((r1.c3 = r3.c3 ORDER BY
> > r1."C 1" ASC NULLS LAST
> > +(4 rows)
> >
> > date_in | s |1 | [0:0]={cstring}
> > date_in which will be used to cast a test to date is not immutable. So
> > the query should't be pushed down. May not be a problem with your
> > patch. Can you please check?
>
> Hi.
>
> It is not related to my change and works as expected. As I see, we have
> expression FuncExprdate(oid = 2029, args=Var ) = Const(type date)
> (date(r3.c5) = '1970-01-17'::date).
> Function is
>
> # select proname, provolatile from pg_proc where oid=2029;
> proname | provolatile
> -+-
> date| i
>
> So it's shippable.
This entry was marked as "Needs review" in the CommitFest app but cfbot
reports the patch no longer applies.
We've marked it as "Waiting on Author". As CommitFest 2022-11 is
currently underway, this would be an excellent time update the patch.
Once you think the patchset is ready for review again, you (or any
interested party) can move the patch entry forward by visiting
https://commitfest.postgresql.org/40/3838/
and changing the status to "Needs review".
Thanks
Ian Barwick
Re: security_context_t marked as deprecated in libselinux 3.1
Alvaro Herrera writes: > On 2020-Aug-14, Michael Paquier wrote: >> On Thu, Aug 13, 2020 at 08:47:28PM -0400, Tom Lane wrote: >>> Should we back-patch that? Usually I figure that people might want >>> to build back PG branches on newer platforms at some point, so that >>> it's useful to apply portability fixes across-the-board. On the >>> other hand, since it's only a compiler warning, maybe it's not worth >>> the trouble. >> Not sure that's worth the trouble as long as people don't complain >> about it directly, and it does not prevent the contrib module to >> work. > FWIW I just had a CI job fail the "warnings" test because of lacking > this patch in the back branches :-) What do you think about > back-patching this to at least 11? No objection to back-patching from me. > I would say 10, but since that one > is going to end soon, it might not be worth much effort. OTOH maybe we > want to backpatch all the way back to 9.2 given the no-warnings policy > we recently acquired. I'm not sure that no-warnings policy extends to stuff as far off the beaten path as sepgsql. However, I won't stand in the way if you want to do that. One point though: if you want to touch v10, I'd suggest waiting till after next week's releases. Unlikely as it is that this'd break anything, I don't think we should risk it in the branch's last release. regards, tom lane
Re: Incorrect include file order in guc-file.l
On Thu, Nov 03, 2022 at 07:19:07PM +0700, John Naylor wrote: > Because it wouldn't compile otherwise, obviously. :-) > > I must have been working on it before bfb9dfd93720 Hehe, my fault then ;p The CI is able to complete without it. Would you mind if it is removed? If you don't want us to poke more at the bear, that's a nit so leaving things as they are is also fine by me. -- Michael signature.asc Description: PGP signature
Re: Use array as object (src/fe_utils/parallel_slot.c)
Michael Paquier writes: > Based on the way the code is written on HEAD, this would be the > correct assumption. Now, calling PQgetCancel() would return NULL for > a connection that we actually ignore in the code a couple of lines > above when it has PGINVALID_SOCKET. So it seems to me that the > suggestion of using "cancelconn", which would be the first valid > connection, rather than always the first connection, which may be > using an invalid socket, is correct, so as we always have our hands > on a way to cancel a command. I came across this commit (52144b6fc) while writing release notes, and I have to seriously question whether it's right yet. The thing that needs to be asked is, if we get a SIGINT in a program using this logic, why would we propagate a cancel to just one of the controlled sessions and not all of them? It looks to me like the original concept was that slot zero would be a "master" connection, such that canceling just that one would have a useful effect. Maybe the current users of parallel_slot.c still use it like that, but I bet it's more likely that the connections are all doing independent things and you really gotta cancel them all if you want out. I suppose maybe this commit improved matters: if you are running N jobs then typing control-C N times (not too quickly) might eventually get you out, by successively canceling the lowest-numbered surviving connection. Previously you could have pounded the key all day and not gotten rid of any but the zero'th task. OTOH, if the connections don't exit but just go back to idle, which seems pretty possible, then it's not clear we've moved the needle at all. Anyway I think this needs rewritten, not just tweaked. The cancel.c infrastructure is really next to useless here since it is only designed with one connection in mind. I'm inclined to think we should only expect the signal handler to set CancelRequested, and then manually issue cancels to all live connections when we see that become set. I'm not proposing reverting 52144b6fc, because I doubt it made anything worse; but I'm thinking of leaving it out of the release notes, because I'm unsure it had any user-visible effect at all. It doesn't look to me like we'd ever get to wait_on_slots unless all the connections are known busy. regards, tom lane
Re: proposal: possibility to read dumped table's name from file
čt 3. 11. 2022 v 5:09 odesílatel Julien Rouhaud napsal:
> Hi,
>
> On Wed, Oct 26, 2022 at 06:26:26AM +0200, Pavel Stehule wrote:
> >
> > út 18. 10. 2022 v 11:33 odesílatel Julien Rouhaud
> > napsal:
> >
> > >
> > > I'm wondering if psql's parse_identifier() could be exported and reused
> > > here
> > > rather than creating yet another version.
> > >
> >
> > I looked there, and I don't think this parser is usable for this purpose.
> > It is very sensitive on white spaces, and doesn't support multi-lines.
> It
> > is designed for support readline tab complete, it is designed for
> > simplicity not for correctness.
>
> Ah, sorry I should have checked more thoroughly. I guess it's ok to have
> another identifier parser for the include file then, as this new one
> wouldn't
> really fit the tab-completion use case.
>
> > > also, is there any reason why this function doesn't call exit_nicely in
> > > case of
> > > error rather than letting each caller do it without any other cleanup?
> > >
> >
> > It is commented few lines up
> >
> > /*
> > * Following routines are called from pg_dump, pg_dumpall and pg_restore.
> > * Unfortunatelly, implementation of exit_nicely in pg_dump and
> pg_restore
> > * is different from implementation of this rutine in pg_dumpall. So
> instead
> > * direct calling exit_nicely we have to return some error flag (in this
> > * case NULL), and exit_nicelly will be executed from caller's routine.
> > */
>
> Oh right, I totally missed it sorry about that!
>
> About the new version, I didn't find any problem with the feature itself so
> it's a good thing!
>
> I still have a few comments about the patch. First, about the behavior:
>
> - is that ok to have just "data" pattern instead of "table_data" or
> something
> like that, since it's supposed to match --exclude-table-data option?
>
done
>
> - the error message are sometimes not super helpful. For instance:
>
> $ echo "include data t1" | pg_dump --filter -
> pg_dump: error: invalid format of filter on line 1: include filter is not
> allowed for this type of object
>
> It would be nice if the error message mentioned "data" rather than a
> generic
> "this type of object". Also, maybe we should quote "include" to outline
> that
> we found this keyword?
>
>
done
> About the patch itself:
> filter.c:
>
> +#include "postgres_fe.h"
> +
> +#include "filter.h"
> +
> +#include "common/logging.h"
>
> the filter.h inclusion should be done with the rest of the includes, in
> alphabetical order.
>
>
done
> +#defineis_keyword_str(cstr, str, bytes) \
> + ((strlen(cstr) == bytes) && (pg_strncasecmp(cstr, str, bytes) ==
> 0))
>
> nit: our guidline is to protect macro arguments with parenthesis. Some
> arguments can be evaluated multiple times but I don't think it's worth
> adding a
> comment for that.
>
>
done
> + * Unfortunatelly, implementation of exit_nicely in pg_dump and pg_restore
> + * is different from implementation of this rutine in pg_dumpall. So
> instead
> + * direct calling exit_nicely we have to return some error flag (in this
>
> typos: s/Unfortunatelly/Unfortunately/ and s/rutine/routine/
> Also, it would probably be better to say "instead of directly calling..."
>
>
done
> +static const char *
> +filter_object_type_name(FilterObjectType fot)
> +{
> + switch (fot)
> + {
> + case FILTER_OBJECT_TYPE_NONE:
> + return "comment or empty line";
> +[...]
> + }
> +
> + return "unknown object type";
> +}
>
> I'm wondering if we should add a pg_unreachable() there, some compilers
> might
> complain otherwise. See CreateDestReceiver() for instance for similar
> pattern.
>
done
>
> + * Emit error message "invalid format of filter file ..."
> + *
> + * This is mostly a convenience routine to avoid duplicating file closing
> code
> + * in multiple callsites.
> + */
> +void
> +log_invalid_filter_format(FilterStateData *fstate, char *message)
>
> nit: invalid format *in* filter file...?
>
changed
>
> +void
> +log_unsupported_filter_object_type(FilterStateData *fstate,
> +
> const char *appname,
> +
> FilterObjectType fot)
> +{
> + PQExpBuffer str = createPQExpBuffer();
> +
> + printfPQExpBuffer(str,
> + "The application \"%s\" doesn't
> support filter for object type \"%s\".",
>
> nit: there shouldn't be uppercase in error messages, especially since this
> will
> be appended to another message by log_invalid_filter_format. I would just
> just
> drop "The application" entirely for brevity.
>
changed
>
> +/*
> + * Release allocated resources for filter
> + */
> +void
> +filter_free(FilterStateData *fstate)
>
> nit: Release allocated resources for *the given* filter?
>
changed
>
> + * Search for keywords (limited to ascii alphabetic characters) in
> + * the passed in line buffer. Returns NULL, when the buffer is empty or
> first
> + * char is not alpha. The length of the found
GUC for temporarily disabling event triggers
In the thread discussing the login event trigger patch it was argued that we want to avoid recommending single-user mode for troubleshooting tasks, and a GUC for temporarily disabling event triggers was proposed. Since the login event trigger patch lost momentum, I've broken out the GUC part into a separate patch to see if there is interest in that part alone, to chip away at situations requiring single-user mode. The patch adds a new GUC, ignore_event_trigger with two option values, 'all' and 'none' (the login event patch had 'login' as well). This can easily be expanded to have the different types of events, or pared down to a boolean on/off. I think it makes more sense to make it more fine-grained but I think there is merit in either direction. If there is interest in this I'm happy to pursue a polished version of this patch. -- Daniel Gustafsson https://vmware.com/ v1-0001-Add-GUC-for-temporarily-disabling-event-triggers.patch Description: Binary data
Re: Tracking last scan time
On Mon, 31 Oct 2022 at 07:36, Dave Page wrote: > FYI, this is not intentional, and I do plan to look into it, however I've > been somewhat busy with pgconfeu, and am travelling for the rest of this > week as well. > Here's a patch to fix this issue. Many thanks to Peter Eisentraut who figured it out in a few minutes after I spent far too long looking down rabbit holes in entirely the wrong place. Thanks for the bug report. > > On Sun, 23 Oct 2022 at 21:09, Robert Treat wrote: > >> On Fri, Oct 14, 2022 at 2:55 PM Dave Page wrote: >> > On Fri, 14 Oct 2022 at 19:16, Andres Freund wrote: >> >> On 2022-10-13 14:38:06 +0100, Dave Page wrote: >> >> > Thanks for that. It looks good to me, bar one comment (repeated 3 >> times in >> >> > the sql and expected files): >> >> > >> >> > fetch timestamps from before the next test >> >> > >> >> > "from " should be removed. >> >> >> >> I was trying to say something with that from, but clearly it wasn't >> >> understandable :). Removed. >> >> >> >> With that I pushed the changes and marked the CF entry as committed. >> > >> > >> > Thanks! >> > >> >> Hey folks, >> >> I was looking at this a bit further (great addition btw) and noticed >> the following behavior (this is a mre of the original testing that >> uncovered this): >> >> pagila=# select * from pg_stat_user_tables ; >> relid | schemaname | relname | seq_scan | last_seq_scan | >> seq_tup_read | idx_scan | last_idx_scan | idx_tup_fetch | n_tup_ins | >> n_tup_upd | n_tup_del | n_tup_hot_upd | n_live_tup | n_dead_tup | >> n_mod_since_analyze | n_ins_since_vacuum | last_vacuum | >> last_autovacuum | last_analyze | last_autoanalyze | vacuum_count | >> autovacuum_count | analyze_count | autoanalyze_count >> >> ---++-+--+---+--+--+---+---+---+---+---+---+++-++-+-+--+--+--+--+---+--- >> (0 rows) >> >> pagila=# create table x (xx int); >> CREATE TABLE >> Time: 2.145 ms >> pagila=# select * from pg_stat_user_tables ; >> relid | schemaname | relname | seq_scan | last_seq_scan | >> seq_tup_read | idx_scan | last_idx_scan | idx_tup_fetch | n_tup_ins | >> n_tup_upd | n_tup_del | n_tup_hot_upd | n_live_tup | n_dead_tup | >> n_mod_since_analyze | n_ins_since_vacuum | last_vacuum | >> last_autovacuum | last_analyze | last_autoanalyze | vacuum_count | >> autovacuum_count | analyze_count | autoanalyze_count >> >> ---++-+--+---+--+--+---+---+---+---+---+---+++-++-+-+--+--+--+--+---+--- >> 16392 | public | x |0 | [null]| >> 0 | [null] | [null]|[null] | 0 | 0 | >> 0 | 0 | 0 | 0 | >> 0 | 0 | [null] | [null] | [null] >> | [null] |0 |0 | 0 | >> 0 >> (1 row) >> >> pagila=# insert into x select 1; >> INSERT 0 1 >> pagila=# select * from pg_stat_user_tables ; >> relid | schemaname | relname | seq_scan | last_seq_scan | >> seq_tup_read | idx_scan | last_idx_scan | idx_tup_fetch | n_tup_ins | >> n_tup_upd | n_tup_del | n_tup_hot_upd | n_live_tup | n_dead_tup | >> n_mod_since_analyze | n_ins_since_vacuum | last_vacuum | >> last_autovacuum | last_analyze | last_autoanalyze | vacuum_count | >> autovacuum_count | analyze_count | autoanalyze_count >> >> ---++-+--++--+--+---+---+---+---+---+---+++-++-+-+--+--+--+--+---+--- >> 16392 | public | x |0 | 1999-12-31 19:00:00-05 | >> 0 | [null] | [null]|[null] | 1 | >> 0 | 0 | 0 | 1 | 0 | >> 1 | 1 | [null] | [null] | >> [null] | [null] |0 |0 | >> 0 | 0 >> (1 row) >> >> Normally we populate "last" columns with a NULL value when the >> corresponding marker is zero, which seems correct in the first query, >> but no longer matches in the second. I can see an argument that this >> is a necessary exception to that rule (I'm not sure I agree with it, >> but I see it) but even in that scenario, ISTM we should
Re: On login trigger: take three
> On 20 Sep 2022, at 16:10, Daniel Gustafsson wrote: > Since the original authors seems to have retired from the patch > (I've only rebased it to try and help) I am inclined to mark it as returned > with feedback. Doing so now since no updates have been posted for quite some time and holes have been poked in the patch. The GUC for temporarily disabling event triggers, to avoid the need for single- user mode during troubleshooting, might however be of interest so I will break that out and post to a new thread. -- Daniel Gustafsson https://vmware.com/
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On 2022-Oct-05, Alvaro Herrera wrote: > I've been giving the patches a look and it caused me to notice two > additional bugs in the same area: > > - FKs in partitions are sometimes marked NOT VALID. This is because of > missing initialization when faking up a Constraint node in > CloneFkReferencing. Easy to fix, have patch, running tests now. I have pushed the fix for this now. > - The feature added by d6f96ed94e73 (ON DELETE SET NULL (...)) is not > correctly propagated. This should be an easy fix also, haven't tried, > need to add a test case. There was no bug here actually: it's true that the struct member is left uninitialized, but in practice that doesn't matter, because the set of columns is propagated separately from the node. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Las navajas y los monos deben estar siempre distantes" (Germán Poo)
Re: heavily contended lwlocks with long wait queues scale badly
On 11/1/22 1:41 PM, Andres Freund wrote: Andres: when you suggested backpatching, were you thinking of the Nov 2022 release or the Feb 2023 release? I wasn't thinking that concretely. Even if we decide to backpatch, I'd be very hesitant to do it in a few days. Yeah this was my thinking (and also why I took a few days to reply given the lack of urgency for this release). It would at least give some more time for others to test it to feel confident that we're not introducing noticeable regressions. I tested with browser etc running, so this is plenty noisy. I used the best of the two pgbench -T21 -P5 tps, after ignoring the first two periods (they're too noisy). I used an ok-ish NVMe SSD, rather than the the expensive one that has "free" fsync. synchronous_commit=on: clients master fix 16 61966202 64 25716 25545 25690131 90240 1024 128556 151487 2048 59417 157050 4096 32252 178823 synchronous_commit=off: clients master fix 16409828 409016 64454257 455804 256 304175 452160 1024 135081 334979 2048 66124 291582 4096 27019 245701 Hm. That's a bigger effect than I anticipated. I guess sc=off isn't actually required, due to the level of concurrency making group commit very effective. This is without an index, serial column or anything. But a quick comparison for just 4096 clients shows that to still be a big difference if I create an serial primary key: master: 26172 fix: 155813 勞 (seeing if my exploding head makes it into the archives). Given the lack of ABI changes (hesitant to say low-risk until after more testing, but seemingly low-risk), I can get behind backpatching esp if we're targeting Feb 2023 so we can tests some more. With my advocacy hat on, it bums me that we may not get as much buzz about this change given it's not in a major release, but 1/ it'll fix an issue that will help users with high-concurrency and 2/ users would be able to perform a simpler update to get the change. Thanks, Jonathan OpenPGP_signature Description: OpenPGP digital signature
Re: ssl tests aren't concurrency safe due to get_free_port()
On 2022-11-02 We 15:09, Andrew Dunstan wrote:
> On 2022-10-17 Mo 10:59, Andrew Dunstan wrote:
>> On 2022-10-04 Tu 01:39, Andrew Dunstan wrote:
>>> On 2022-10-02 Su 12:49, Andres Freund wrote:
2) Use a lockfile containing a pid to protect the choice of a port within a
build directory. Before accepting a port get_free_port() would check if
the
a lockfile exists for the port and if so, if the test using it is still
alive. That will protect against racyness between multiple tests
inside a
build directory, but won't protect against races between multiple builds
running concurrently on a machine (e.g. on a buildfarm host)
>>> I think this is the right solution. To deal with the last issue, the
>>> lockdir should be overrideable, like this:
>>>
>>>
>>> my $port_lockdir = $ENV{PG_PORT_LOCKDIR} || $build_dir;
>>>
>>>
>>> Buildfarm animals could set this, probably to the global lockdir (see
>>> run_branches.pl). Prior to that, buildfarm owners could do that manually.
>>>
>>>
>> The problem here is that Cluster.pm doesn't have any idea where the
>> build directory is, or even if there is one present at all.
>>
>> meson does appear to let us know that, however, with MESON_BUILD_ROOT,
>> so probably the best thing would be to use PG_PORT_LOCKDIR if it's set,
>> otherwise MESON_BUILD_ROOT if it's set, otherwise the tmp_check
>> directory. If we want to backport to the make system we could export
>> top_builddir somewhere.
>>
>>
> Here's a patch which I think does the right thing.
>
>
>
Updated with a couple of thinkos fixed.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index d80134b26f..aceca353d3 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -93,9 +93,9 @@ use warnings;
use Carp;
use Config;
-use Fcntl qw(:mode);
+use Fcntl qw(:mode :flock :seek O_CREAT O_RDWR);
use File::Basename;
-use File::Path qw(rmtree);
+use File::Path qw(rmtree mkpath);
use File::Spec;
use File::stat qw(stat);
use File::Temp ();
@@ -109,7 +109,7 @@ use Time::HiRes qw(usleep);
use Scalar::Util qw(blessed);
our ($use_tcp, $test_localhost, $test_pghost, $last_host_assigned,
- $last_port_assigned, @all_nodes, $died);
+ $last_port_assigned, @all_nodes, $died, $portdir);
# the minimum version we believe to be compatible with this package without
# subclassing.
@@ -140,6 +140,27 @@ INIT
# Tracking of last port value assigned to accelerate free port lookup.
$last_port_assigned = int(rand() * 16384) + 49152;
+
+ # Set the port lock directory
+
+ # If we're told to use a directory (e.g. from a buildfarm client)
+ # explicitly, use that
+ $portdir = $ENV{PG_TEST_PORT_DIR};
+ # Otherwise, try to use a directory at the top of the build tree
+ if (! $portdir && $ENV{MESON_BUILD_ROOT})
+ {
+ $portdir = $ENV{MESON_BUILD_ROOT} . '/portlock'
+ }
+ elsif (! $portdir && ($ENV{TESTDATADIR} || "") =~ /\W(src|contrib)\W/p)
+ {
+ my $dir = ${^PREMATCH};
+ $portdir = "$dir/portlock" if $dir;
+ }
+ # As a last resort use a directory under tmp_check
+ $portdir ||= $PostgreSQL::Test::Utils::tmp_check . '/portlock';
+ $portdir =~ s!\\!/!g;
+ # Make sure the directory exists
+ mkpath($portdir) unless -d $portdir;
}
=pod
@@ -1505,6 +1526,7 @@ sub get_free_port
last;
}
}
+ $found = _reserve_port($port) if $found;
}
}
@@ -1535,6 +1557,38 @@ sub can_bind
return $ret;
}
+# Internal routine to reserve a port number
+# Returns 1 if successful, 0 if port is already reserved.
+sub _reserve_port
+{
+ my $port = shift;
+ # open in rw mode so we don't have to reopen it and lose the lock
+ sysopen(my $portfile, "$portdir/$port.rsv", O_RDWR|O_CREAT)
+ || die "opening port file";
+ # take an exclusive lock to avoid concurrent access
+ flock($portfile, LOCK_EX) || die "locking port file";
+ # see if someone else has or had a reservation of this port
+ my $pid = <$portfile>;
+ chomp $pid;
+ if ($pid +0 > 0)
+ {
+ if (kill 0, $pid)
+ {
+ # process exists and is owned by us, so we can't reserve this port
+ close($portfile);
+ return 0;
+ }
+ }
+ # All good, go ahead and reserve the port, first rewind and truncate.
+ # If truncation fails it's not a tragedy, it just might leave some
+ # trailing junk in the file that won't affect us.
+ seek($portfile, 0, SEEK_SET);
+ truncate($portfile, 0);
+ print $portfile "$$\n";
+ close($portfile);
+ return 1;
+}
+
# Automatically shut down any still-running nodes (in the same order the nodes
# were created in) when the test script exits.
END
Re: Fix proposal for comparaison bugs in PostgreSQL::Version
On Tue, Jun 28, 2022 at 06:17:40PM -0400, Andrew Dunstan wrote: > Nice catch, but this looks like massive overkill. I think we can very > simply fix the test in just a few lines of code, instead of a 190 line > fix and a 130 line TAP test. > > It was never intended to be able to compare markers like rc1 vs rc2, and > I don't see any need for it. If you can show me a sane use case I'll > have another look, but right now it seems quite unnecessary. > > Here's my proposed fix. > > diff --git a/src/test/perl/PostgreSQL/Version.pm > b/src/test/perl/PostgreSQL/Version.pm > index 8f70491189..8d4dbbf694 100644 > --- a/src/test/perl/PostgreSQL/Version.pm Is this still an outstanding issue ? -- Justin
Re: security_context_t marked as deprecated in libselinux 3.1
On 2020-Aug-14, Michael Paquier wrote: > On Thu, Aug 13, 2020 at 08:47:28PM -0400, Tom Lane wrote: > > Should we back-patch that? Usually I figure that people might want > > to build back PG branches on newer platforms at some point, so that > > it's useful to apply portability fixes across-the-board. On the > > other hand, since it's only a compiler warning, maybe it's not worth > > the trouble. > > Not sure that's worth the trouble as long as people don't complain > about it directly, and it does not prevent the contrib module to > work. FWIW I just had a CI job fail the "warnings" test because of lacking this patch in the back branches :-) What do you think about back-patching this to at least 11? I would say 10, but since that one is going to end soon, it might not be worth much effort. OTOH maybe we want to backpatch all the way back to 9.2 given the no-warnings policy we recently acquired. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "I must say, I am absolutely impressed with what pgsql's implementation of VALUES allows me to do. It's kind of ridiculous how much "work" goes away in my code. Too bad I can't do this at work (Oracle 8/9)." (Tom Allison) http://archives.postgresql.org/pgsql-general/2007-06/msg00016.php
Re: remap the .text segment into huge pages at run time
Hi,
On 2022-11-02 13:32:37 +0700, John Naylor wrote:
> It's been known for a while that Postgres spends a lot of time translating
> instruction addresses, and using huge pages in the text segment yields a
> substantial performance boost in OLTP workloads [1][2].
Indeed. Some of that we eventually should address by making our code less
"jumpy", but that's a large amount of work and only going to go so far.
> The difficulty is,
> this normally requires a lot of painstaking work (unless your OS does
> superpage promotion, like FreeBSD).
I still am confused by FreeBSD being able to do this without changing the
section alignment to be big enough. Or is the default alignment on FreeBSD
large enough already?
> I found an MIT-licensed library "iodlr" from Intel [3] that allows one to
> remap the .text segment to huge pages at program start. Attached is a
> hackish, Meson-only, "works on my machine" patchset to experiment with this
> idea.
I wonder how far we can get with just using the linker hints to align
sections. I know that the linux folks are working on promoting sufficiently
aligned executable pages to huge pages too, and might have succeeded already.
IOW, adding the linker flags might be a good first step.
> 0001 adapts the library to our error logging and GUC system. The overview:
>
> - read ELF info to get the start/end addresses of the .text segment
> - calculate addresses therein aligned at huge page boundaries
> - mmap a temporary region and memcpy the aligned portion of the .text
> segment
> - mmap aligned start address to a second region with huge pages and
> MAP_FIXED
> - memcpy over from the temp region and revoke the PROT_WRITE bit
Would mremap()'ing the temporary region also work? That might be simpler and
more robust (you'd see the MAP_HUGETLB failure before doing anything
irreversible). And you then might not even need this:
> The reason this doesn't "saw off the branch you're standing on" is that the
> remapping is done in a function that's forced to live in a different
> segment, and doesn't call any non-libc functions living elsewhere:
>
> static void
> __attribute__((__section__("lpstub")))
> __attribute__((__noinline__))
> MoveRegionToLargePages(const mem_range * r, int mmap_flags)
This would likely need a bunch more gating than the patch, understandably,
has. I think it'd faily horribly if there were .text relocations, for example?
I think there are some architectures that do that by default...
> 0002 is my attempt to force the linker's hand and get the entire text
> segment mapped to huge pages. It's quite a finicky hack, and easily broken
> (see below). That said, it still builds easily within our normal build
> process, and maybe there is a better way to get the effect.
>
> It does two things:
>
> - Pass the linker -Wl,-zcommon-page-size=2097152
> -Wl,-zmax-page-size=2097152 which aligns .init to a 2MB boundary. That's
> done for predictability, but that means the next 2MB boundary is very
> nearly 2MB away.
Yep. FWIW, my notes say
# align sections to 2MB boundaries for hugepage support
# bfd and gold linkers:
# -Wl,-zmax-page-size=0x20 -Wl,-zcommon-page-size=0x20
# lld:
# -Wl,-zmax-page-size=0x20 -Wl,-z,separate-loadable-segments
# then copy binary to tmpfs mounted with -o huge=always
I.e. with lld you need slightly different flags
-Wl,-z,separate-loadable-segments
The meson bit should probably just use
cc.get_supported_link_arguments([
'-Wl,-zmax-page-size=0x20',
'-Wl,-zcommon-page-size=0x20',
'-Wl,-zseparate-loadable-segments'])
Afaict there's really no reason to not do that by default, allowing kernels
that can promote to huge pages to do so.
My approach to forcing huge pages to be used was to then:
# copy binary to tmpfs mounted with -o huge=always
> - Add a "cold" __asm__ filler function that just takes up space, enough to
> push the end of the .text segment over the next aligned boundary, or to
> ~8MB in size.
I don't understand why this is needed - as long as the pages are aligned to
2MB, why do we need to fill things up on disk? The in-memory contents are the
relevant bit, no?
> Since the front is all-cold, and there is very little at the end,
> practically all hot pages are now remapped. The biggest problem with the
> hackish filler function (in addition to maintainability) is, if explicit
> huge pages are turned off in the kernel, attempting mmap() with MAP_HUGETLB
> causes complete startup failure if the .text segment is larger than 8MB.
I would expect MAP_HUGETLB to always fail if not enabled in the kernel,
independent of the .text segment size?
> +/* Callback for dl_iterate_phdr to set the start and end of the .text
> segment */
> +static int
> +FindMapping(struct dl_phdr_info *hdr, size_t size, void *data)
> +{
> + ElfW(Shdr) text_section;
> + FindParams *find_params = (FindParams *) data;
> +
> + /*
> + * We are only interested in the mapping matching the main executable.
> + *
remove unnecessary assignment to tmask in DecodeDateTime
Hi, I was looking at the code in DecodeDateTime() around line 1382: tmask = DTK_M(type); In case type is UNKNOWN_FIELD, the above macro would shift 1 left 31 bits which cannot be represented in type 'int'. Looking down in the same function, we can see that tmask is assigned for every legitimate case. If my understanding is correct, please take a look at the proposed patch. Thanks tmask-decode-date-time.patch Description: Binary data
new option to allow pg_rewind to run without full_page_writes
Hello,
Currently pg_rewind refuses to run if full_page_writes is off. This is
to prevent it to run into a torn page during operation.
This is usually a good call, but some file systems like ZFS are
naturally immune to torn page (maybe btrfs too, but I don't know for
sure for this one).
Having the option to use pg_rewind without the cost associated with
full_page_writes when using a system immune to torn page is beneficial:
increased performance and more compact WAL.
This patch adds a new option "--no-ensure-full-page-writes" to pg_rewind
for this situation, as well as patched documentation.
Regards,
Jeremie Grauer
From 4f973b7e2d4bd9c04b6b6ce2c825dfdab4dbad11 Mon Sep 17 00:00:00 2001
From: Jeremie Grauer
Date: Thu, 3 Nov 2022 16:23:49 +0100
Subject: [PATCH] adds the option --no-ensure-full-page-writes to pg_rewind
---
doc/src/sgml/ref/pg_rewind.sgml | 25 +-
src/bin/pg_rewind/libpq_source.c | 44 ++--
src/bin/pg_rewind/pg_rewind.c| 43 ++-
src/bin/pg_rewind/pg_rewind.h| 1 +
4 files changed, 75 insertions(+), 38 deletions(-)
diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index 69d6924b3a..3dbdb503cd 100644
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -99,7 +99,11 @@ PostgreSQL documentation
in postgresql.conf or data checksums enabled when
the cluster was initialized with initdb. Neither of these
are currently on by default.
- must also be set to on, but is enabled by default.
+ should also be set to on (which is the default) except when the
+ underlying system is not succeptible to torn pages. ZFS filesystem for instance doesn't
+ need this parameter. If you are running such a system, you need to add the option
+ --no-enforce-full-page-writes else pg_rewind will refuse
+ to run.
@@ -283,6 +287,25 @@ PostgreSQL documentation
+
+ --no-ensure-full-page-writes
+
+
+pg_rewind by default requires that
+is set to on. This ensures that pg_rewind do
+not run into torn pages while running. This option is necessary most of
+the time since very few filesystems enforce the flush of full page to disk.
+One such system is ZFS where the page is always flushed to disk in its
+entirety. Other storage system may also have the same warranty. This option
+is here to allow pg_rewind to run on such systems without enforcing that
+ is on. It's important to know what
+you are doing before setting this setting because in case your system does
+not really warrants that the page is persisted to disk in its entirety, you
+may run into data corruption.
+
+
+
+
-V
--version
diff --git a/src/bin/pg_rewind/libpq_source.c b/src/bin/pg_rewind/libpq_source.c
index 011c9cce6e..7b0c1d54f2 100644
--- a/src/bin/pg_rewind/libpq_source.c
+++ b/src/bin/pg_rewind/libpq_source.c
@@ -132,26 +132,32 @@ init_libpq_conn(PGconn *conn)
PQclear(res);
/*
- * Also check that full_page_writes is enabled. We can get torn pages if
- * a page is modified while we read it with pg_read_binary_file(), and we
- * rely on full page images to fix them.
+ * If --no-ensure-full-page-write is false (the default), check that
+ * full_page_writes is enabled. The goal here is to not run into a torn
+ * pages. Torn page can happend if a page is modified while we read it
+ * with pg_read_binary_file(). Some file systems are immune to torn page,
+ * this is why there is an option to disable this check. Outside of those
+ * specific systems, we rely on full_page_writes to avoid torn page.
*/
- str = run_simple_query(conn, "SHOW full_page_writes");
- if (strcmp(str, "on") != 0)
- pg_fatal("full_page_writes must be enabled in the source server");
- pg_free(str);
-
- /* Prepare a statement we'll use to fetch files */
- res = PQprepare(conn, "fetch_chunks_stmt",
- "SELECT path, begin,\n"
- " pg_read_binary_file(path, begin, len, true) AS chunk\n"
- "FROM unnest ($1::text[], $2::int8[], $3::int4[]) as x(path, begin, len)",
- 3, NULL);
-
- if (PQresultStatus(res) != PGRES_COMMAND_OK)
- pg_fatal("could not prepare statement to fetch file contents: %s",
- PQresultErrorMessage(res));
- PQclear(res);
+ if (!no_ensure_full_page_writes)
+ {
+ str = run_simple_query(conn, "SHOW full_page_writes");
+ if (strcmp(str, "on") != 0)
+ pg_fatal("full_page_writes must be enabled in the source server");
+ pg_free(str);
+
+ /* Prepare a statement we'll use to fetch files */
+ res = PQprepare(conn, "fetch_chunks_stmt",
+ "SELECT path, begin,\n"
+ " pg_read_binary_file(path, begin, len, true) AS chunk\n"
+ "FROM unnest ($1::text[], $2::int8[], $3::int4[]) as x(path, begin, len)",
+ 3, NULL);
+
+ if (PQresultStatus(res) != PGRES_COMMAND_OK)
+ pg_fatal("could not
Re: Add the ability to limit the amount of memory that can be allocated to backends.
On Tue, 2022-10-25 at 11:49 -0400, Reid Thompson wrote: > Hi Arne, > > On Mon, 2022-10-24 at 15:27 +, Arne Roland wrote: > > Hello Reid, > > > > could you rebase the patch again? It doesn't apply currently > > (http://cfbot.cputube.org/patch_40_3867.log). Thanks! > > rebased patches attached. Rebased to current. Add a couple changes per conversation with D Christensen (include units in field name, group field with backend_xid and backend_xmin fields in pg_stat_activity view, rather than between query_id and query) -- Reid Thompson Senior Software Engineer Crunchy Data, Inc. reid.thomp...@crunchydata.com www.crunchydata.com From 9cf35c79be107feedb63f6f674ac9d2347d1875e Mon Sep 17 00:00:00 2001 From: Reid Thompson Date: Sat, 4 Jun 2022 22:23:59 -0400 Subject: [PATCH 2/2] Add the ability to limit the amount of memory that can be allocated to backends. This builds on the work that adds backend memory allocated to pg_stat_activity. Add GUC variable max_total_backend_memory. Specifies a limit to the amount of memory (in MB) that may be allocated to backends in total (i.e. this is not a per user or per backend limit). If unset, or set to 0 it is disabled. It is intended as a resource to help avoid the OOM killer on LINUX and manage resources in general. A backend request that would push the total over the limit will be denied with an out of memory error causing that backend's current query/transaction to fail. Due to the dynamic nature of memory allocations, this limit is not exact. If within 1.5MB of the limit and two backends request 1MB each at the same time both may be allocated, and exceed the limit. Further requests will not be allocated until dropping below the limit. Keep this in mind when setting this value. This limit does not affect auxiliary backend processes. Backend memory allocations are displayed in the pg_stat_activity view. --- doc/src/sgml/config.sgml | 26 src/backend/storage/ipc/dsm_impl.c| 12 ++ src/backend/utils/activity/backend_status.c | 111 +- src/backend/utils/misc/guc_tables.c | 11 ++ src/backend/utils/misc/postgresql.conf.sample | 3 + src/backend/utils/mmgr/aset.c | 17 +++ src/backend/utils/mmgr/generation.c | 9 ++ src/backend/utils/mmgr/slab.c | 8 ++ src/include/utils/backend_status.h| 2 + 9 files changed, 198 insertions(+), 1 deletion(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 559eb898a9..5762999fa5 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -2079,6 +2079,32 @@ include_dir 'conf.d' + + max_total_backend_memory (integer) + + max_total_backend_memory configuration parameter + + + + +Specifies a limit to the amount of memory (MB) that may be allocated to +backends in total (i.e. this is not a per user or per backend limit). +If unset, or set to 0 it is disabled. A backend request that would +push the total over the limit will be denied with an out of memory +error causing that backend's current query/transaction to fail. Due to +the dynamic nature of memory allocations, this limit is not exact. If +within 1.5MB of the limit and two backends request 1MB each at the same +time both may be allocated, and exceed the limit. Further requests will +not be allocated until dropping below the limit. Keep this in mind when +setting this value. This limit does not affect auxiliary backend +processes . Backend memory +allocations (backend_allocated_bytes) are displayed in +the pg_stat_activity +view. + + + + diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c index eae03159c3..aaf74e9486 100644 --- a/src/backend/storage/ipc/dsm_impl.c +++ b/src/backend/storage/ipc/dsm_impl.c @@ -254,6 +254,10 @@ dsm_impl_posix(dsm_op op, dsm_handle handle, Size request_size, return true; } + /* Do not exceed maximum allowed memory allocation */ + if (op == DSM_OP_CREATE && exceeds_max_total_bkend_mem(request_size)) + return false; + /* * Create new segment or open an existing one for attach. * @@ -525,6 +529,10 @@ dsm_impl_sysv(dsm_op op, dsm_handle handle, Size request_size, int flags = IPCProtection; size_t segsize; + /* Do not exceed maximum allowed memory allocation */ + if (op == DSM_OP_CREATE && exceeds_max_total_bkend_mem(request_size)) + return false; + /* * Allocate the memory BEFORE acquiring the resource, so that we don't * leak the resource if memory allocation fails. @@ -719,6 +727,10 @@ dsm_impl_windows(dsm_op op, dsm_handle handle, Size request_size, return true; } + /* Do not exceed maximum allowed memory allocation */ + if (op == DSM_OP_CREATE && exceeds_max_total_bkend_mem(request_size)) +
Re: Privileges on PUBLICATION
On 03.11.22 01:43, Antonin Houska wrote: Peter Eisentraut wrote: The CF entry is about privileges on publications. Please rebase that patch and repost it so that the CF app and the CF bot are up to date. The rebased patch (with regression tests added) is attached here. Some preliminary discussion: What is the upgrade strategy? I suppose the options are either that publications have a default acl that makes them publicly accessible, thus preserving the existing behavior by default, or pg_dump would need to create additional GRANT statements when upgrading from pre-PG16. I don't see anything like either of these mentioned in the patch. What is your plan? You might be interested in this patch, which relates to yours: https://commitfest.postgresql.org/40/3955/ Looking at your patch, I would also like to find a way to refactor away the ExecGrant_Publication() function. I'll think about that. I think you should add some tests under src/test/regress/ for the new GRANT and REVOKE statements, just to have some basic coverage that it works. sql/publication.sql would be appropriate, I think.
Re: Speed up transaction completion faster after many relations are accessed in a transaction
Hi David, This is review of speed up releasing of locks patch. Contents & Purpose: Subject is missing in patch. It would have been easier to understand purpose had it been included. Included in the patch are change in README, but no new tests are included.. Initial Run: The patch applies cleanly to HEAD. The regression tests all pass successfully against the new patch. Nitpicking & conclusion: I don't see any performance improvement in tests. Lots of comments were removed which were not fully replaced. Change of log level for ReleaseLockIfHeld: failed from warning to panic is mystery. Change in readme doesn't look right. `Any subsequent lockers are share lockers wait waiting for the VXID to terminate via some other method) is for deadlock`. This sentence could be rewritten. Also more comments could be added to explain new methods added. Thanks, Ankit
Re: parse partition strategy string in gram.y
Pushed this. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Re: [BUG] Logical replica crash if there was an error in a function.
Amit Kapila writes: > LGTM. I don't know if it is a good idea to omit the test case for this > scenario. If required, we can reuse the test case from Sawada-San's > patch in the email [1]. I don't think the cost of that test case is justified by the tiny probability that it'd ever catch anything. If we were just adding a query or two to an existing scenario, that could be okay; but spinning up and syncing a whole new primary and standby database is *expensive* when you multiply it by the number of times developers and buildfarm animals are going to run the tests in the future. There's also the little issue that I'm not sure it would actually detect a problem if we had one. The case is going to fail, and what we want to know is just how messily it fails, and I think the TAP infrastructure isn't very sensitive to that ... especially if the test isn't even checking for specific error messages. regards, tom lane
Re: Add palloc_aligned() to allow arbitrary power of 2 memory alignment
Hi! Part of the work that Thomas mentions in [1], regarding Direct I/O, > has certain requirements that pointers must be page-aligned. > > I've attached a patch which implements palloc_aligned() and > MemoryContextAllocAligned() ... > I've done a quick look and the patch is looks good to me. Let's add tests for these functions, should we? If you think this is an overkill, feel free to trim tests for your taste. -- Best regards, Maxim Orlov. v1-0002-Add-tests-for-palloc_aligned.patch Description: Binary data v1-0001-palloc_aligned.patch Description: Binary data
Re: Add explicit casts in four places to simplehash.h
David Geier writes: > What about, while not being strictly necessary for PostgreSQL itself, > also adding such casts to simplehash.h so that it can be used in code > where -Wc++-compat is enabled? Seems reasonable, so done (I fixed one additional spot you missed). The bigger picture here is that we do actually endeavor to keep (most of) our headers C++ clean, but our tool cpluspluscheck misses these problems because it doesn't try to use these macros. I wonder whether there is a way to do better. regards, tom lane
Re: Pluggable toaster
Hi, Setting TOAST for table and database is a subject for discussion. There is already default Toaster. Also, there is not much sense in setting Jsonb Toaster as default even for table, do not say database, because table could contain other TOASTable columns not of Json type. To be able to set custom Toaster as default for table you have to make it work with ALL TOASTable datatypes - which leads to lots and lots lines of code, complexity and difficulties supporting such custom Toaster. Custom Toasters are meant to be rather small and have specialty in some tricky datatypes or workflow. Custom Toasters will work with Extended storage, but as I answered in previous email - there is no much use of it, because they would deal with compressed data. >No, encryption is an excellent example of what a TOASTer should NOT >do. If you are interested in encryption consider joining the "Moving >forward with TDE" thread [2]. I'm not working with encryption, so maybe it is really out of scope example. Anyway, compression and dealing with data with known internal structure or some special requirements lile geometric data in PostGIS - for example, custom PostGIS Toaster gives considerable performance boost. >But should we really distinguish INSERT and UPDATE cases on this API >level? It seems to me that TableAM just inserts new tuples. It's >TOASTers job to figure out whether similar values existed before and >should or shouldn't be reused. Additionally a particular TOASTer can >reuse old values between _different_ rows, potentially even from >different tables. Another reason why in practice there is little use >of knowing whether the data is INSERTed or UPDATEd. For TOASTer you SHOULD distinguish insert and update operations, really. Because for TOASTed data these operations affect many tuples, and AM does know which of them were updated and which were not - that's very serious limitation of current TOAST, and TOAST mechanics shoud care about updating affected tuples only instead of marking whole record dead and inserting new one. This is also an argument for not using EXTENDED storage mode - because with compressed data you do not have such choice, you should drop the whole record. Correctly implemented UPDATE for TOAST boosts performance and considerably decreases size of TOAST tables along with WAL size. This is not a question, an UPDATE operation for TOASTed data is a must - consider updating 1 Gb TOASTed record - with current TOAST you would finish having 2 1 Gb records in a table, one of them dead, and 2 Gb in WAL. With update you would have the same 1 Gb record and only update diff in WAL. >Users should be able to DROP extension. I seriously doubt that the >patch is going to be accepted as long as it has this limitation. There is a mention in documentation and previous discussion that this operation would lead to loss of data TOASTed with this custom Toaster. It was stated as an issue and subject for further duscucssion in previous emails. > > -- Regards, Nikita Malakhov Postgres Professional https://postgrespro.ru/
Re: Moving forward with TDE
Hi David, > Working with Stephen, I am attempting to pick up some of the work that > was left off with TDE and the key management infrastructure. I have > rebased Bruce's KMS/TDE patches as they existed on the > https://wiki.postgresql.org/wiki/Transparent_Data_Encryption wiki > page, which are enclosed in this email. I'm happy to see that the TDE effort was picked up. > I would love to open a discussion about how to move forward and get > some of these features built out. The historical threads here are > quite long and complicated; is there a "current state" other than the > wiki that reflects the general thinking on this feature? Any major > developments in direction that would not be reflected in the code from > May 2021? The patches seem to be well documented and decomposed in small pieces. That's good. Unless somebody in the community remembers open questions/issues with TDE that were never addressed I suggest simply iterating with our usual testing/reviewing process. For now I'm going to change the status of the CF entry [1] to "Waiting for Author" since the patchset doesn't pass the CI [2]. One limitation of the design described on the wiki I see is that it seems to heavily rely on AES: > We will use Advanced Encryption Standard (AES) [4]. We will offer three key > length options (128, 192, and 256-bits) selected at initdb time with > --file-encryption-method (there doesn't seem to be any mention of the hash/MAC algorithms, that's odd). In the future we should be able to add the support of alternative algorithms. The reason is that the algorithms can become weak every 20 years or so, and the preferred algorithms may also depend on the region. This should NOT be implemented in this particular patchset, but the design shouldn't prevent from implementing this in the future. [1]: https://commitfest.postgresql.org/40/3985/ [2]: http://cfbot.cputube.org/ -- Best regards, Aleksander Alekseev
Re: real/float example for testlibpq3
Peter Eisentraut writes: > On 01.11.22 09:15, Tom Lane wrote: >> Agreed that the libpq manual is not the place for this, but I feel >> like it will also be clutter in "Data Types". Perhaps we should >> invent a new appendix or the like? Somewhere near the wire protocol >> docs seems sensible. > Would that clutter the protocol docs? ;-) I said "near", not "in". At the time I was thinking "new appendix", but I now recall that the wire protocol docs are not an appendix but a chapter in the Internals division. So that doesn't seem like quite the right place anyway. Perhaps a new chapter under "IV. Client Interfaces" is the right place? If we wanted to get aggressive, we could move most of the nitpicky details about datatype text formatting (e.g., the array quoting rules) there too. I'm not set on that, but it'd make datatype.sgml smaller which could hardly be a bad thing. > I suppose figuring out exactly where to put it and how to mark it up, > etc., in a repeatable fashion is part of the job here. Yup. regards, tom lane
Re: Pluggable toaster
Hi Nikita, Please, avoid top-posting [1]. > Toaster is set for the table column. Each TOASTable column could have > a different Toaster, so column option is the most obvious place to add it. This is a major limitation. IMO the user should be able to set a custom TOASTer for the entire table as well. Ideally - for the entire database too. This could be implemented entirely on the syntax level, the internals of the patch are not going to be affected. > >1.2. That's odd. TOAST should work for EXTENDED and MAIN storage > >strategies as well. On top of that, why should custom TOASTers have > >any knowledge of the default four-stage algorithm and the storage > >strategies? If the storage strategy is actually ignored, it shouldn't > >be used in the syntax. > > EXTENDED storage strategy means that TOASTed value is compressed > before being TOASTed, so no knowledge of its internals could be of any > use. EXTERNAL strategy means that value is being TOASTed in original > form. Storage strategy is the thing internal to AM used, and TOAST > mechanics is not meant to interfere with it. Again, STORAGE EXTERNAL > explicitly shows that value will be stored out-of-line. Let me rephrase. Will the custom TOASTers work only for EXTERNAL storage strategy or this is just a syntax? > >2. Although it's possible to implement some encryption in a TOASTer I > >don't think the documentation should advertise this. > > It is a good example of what could the Toaster be responsible for No, encryption is an excellent example of what a TOASTer should NOT do. If you are interested in encryption consider joining the "Moving forward with TDE" thread [2]. > >3.1. I believe we should rename this to something like `struct > >ToastImpl`. The `Tsr` abbreviation only creates confusion, and this is > >not a routine. > > It was done similar to Table AM Routine (please check Pluggable > Storage API), along with some other decisions. OK, then maybe we shall keep the "Routine" part for consistency. I still don't like the "Tsr" abbreviation though and find it confusing. > It is not clear because current TOAST mechanics does not have UPDATE > functionality - it doesn't actually update TOASTed value, it marks this value > "dead" and inserts a new one. This is the cause of TOAST tables bloating > that is being complained about by many users. Update method is provided > for implementation of UPDATE operation. But should we really distinguish INSERT and UPDATE cases on this API level? It seems to me that TableAM just inserts new tuples. It's TOASTers job to figure out whether similar values existed before and should or shouldn't be reused. Additionally a particular TOASTer can reuse old values between _different_ rows, potentially even from different tables. Another reason why in practice there is little use of knowing whether the data is INSERTed or UPDATEd. > I already answered this question, maybe the answer was not very clear. > This is just an extension entry point, because for some more advanced > functionality existing pre-defined set of methods would be not enough, i.e. > special Toasters for complex datatypes like JSONb, that have complex > internal structure and may require additional ways to interact with it along > toast/detoast/update/delete. Maybe so, but it doesn't change the fact that the user documentation should clearly describe the interface and its usage. > These too. About free() method - Toasters are not meant to be deleted, > we mentioned this several times. They exist once they are created as long > as the DB itself. Have I answered your question? Users should be able to DROP extension. I seriously doubt that the patch is going to be accepted as long as it has this limitation. [1]: https://wiki.postgresql.org/wiki/Mailing_Lists#Email_etiquette_mechanics [2]: https://www.postgresql.org/message-id/flat/CAOxo6XJh95xPOpvTxuP_kiGRs8eHcaNrmy3kkzWrzWxvyVkKkQ%40mail.gmail.com -- Best regards, Aleksander Alekseev
Re: real/float example for testlibpq3
On 01.11.22 09:15, Tom Lane wrote: Peter Eisentraut writes: If we want to start documenting the binary format for all data types, it should go into the "Data Types" chapter. There, we already document the text format in great detail, so putting the binary format in there as well would make sense. Agreed that the libpq manual is not the place for this, but I feel like it will also be clutter in "Data Types". Perhaps we should invent a new appendix or the like? Somewhere near the wire protocol docs seems sensible. Would that clutter the protocol docs? ;-) I suppose figuring out exactly where to put it and how to mark it up, etc., in a repeatable fashion is part of the job here.
Re: Incorrect include file order in guc-file.l
On Thu, Nov 3, 2022 at 6:40 PM Michael Paquier wrote: > > On Thu, Nov 03, 2022 at 12:40:19PM +0700, John Naylor wrote: > > On Wed, Nov 2, 2022 at 1:01 PM Julien Rouhaud wrote: > >> Agreed, it's apparently an oversight in dac048f71eb. +1 for the patch. > > > > I've pushed this, thanks! > > Thanks for the commit. I've wanted to get it done yesterday but life > took over faster than that. Before committing the change, there is > something I have noticed though: this header does not seem to be > necessary at all and it looks that there is nothing in guc-file.l that > needs it. Why did you add it in dac048f to begin with? Because it wouldn't compile otherwise, obviously. :-) I must have been working on it before bfb9dfd93720 -- John Naylor EDB: http://www.enterprisedb.com
Re: Commit fest 2022-11
On Thu, Nov 03, 2022 at 06:48:34PM +0900, Ian Lawrence Barwick wrote: > I am wondering what the best thing to do with cases like this is: > > https://commitfest.postgresql.org/40/3977/ > > where there were multiple patches in the original post, and some but not all > were applied - so those ones are now failing to apply in the cfbot. Should we > request the author to update the thread with those patches which are > still pending? A case-by-case analysis is usually adapted, but if subject is not over yet, and only a portion of the patches have been addressed, keeping it around is the best course of action IMO. The author and/or reviewer may decide otherwise later on, or the patch could always be revisited at the end of the CF and marked as committed, though it would be good to update the thread to reflect that. By experience, it does not happen that often. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Wed, Nov 02, 2022 at 09:06:02PM +0800, Julien Rouhaud wrote: > Maybe one alternative approach would be to keep modifying the current test, > and > only do the split when committing the first part? The split itself should be > easy and mechanical: just remove everything unneeded (different file names > including the file name argument from the add_(hba|ident)_line, inclusion > directive and so on). Even if the diff is then difficult to read it's not > really a problem as it should have already been reviewed. I have not reviewed the test part much yet, TBH. The path manipulations because of WIN32 are a bit annoying. I was wondering if we could avoid all that by using the basenames, as one option. >> I have spent some time starting at the logic today of the whole, and >> GetConfFilesInDir() is really the first thing that stood out. I am >> not sure that it makes much sense to keep that in guc-file.c, TBH, >> once we feed it into hba.c. Perhaps we could put the refactored >> routine (and AbsoluteConfigLocation as a side effect) into a new file >> in misc/? > > Yes I was wondering about it too. A new fine in misc/ looks sensible. conffiles.c is the best thing I could come up with, conf.c and config.c being too generic and these are routines that work on configuration files, so.. >> Your patch proposes a different alternative, which is to pass down the >> memory context created in tokenize_auth_file() down to the callers >> with tokenize_file_with_context() dealing with all the internals. >> process_included_auth_file() is different extension of that. >> This may come down to a matter of taste, but could be be cleaner to >> take an approach similar to tokenize_inc_file() and just create a copy >> of the AuthToken list coming from a full file and append it to the >> result rather than passing around the memory context for the lines? >> This would lead to some simplifications, it seems, at least with the >> number of arguments passed down across the layers. > > I guess it goes in the same direction as 8fea86830e1 where infrastructure to > copy AuthTokens was added, I'm fine either way. I won't hide that I am trying to make the changes a maximum incremental for this thread so as the final part is easy-ish. >> The addition of a check for the depth in two places seems unnecessary, >> and it looks like we should do this kind of check in only one place. > > I usually prefer to add a maybe unnecessary depth check since it's basically > free rather than risking an unfriendly stack size error (including in possible > later refactoring), but no objection to get rid of it. The depth check is a good idea, though I guess that there is an argument for having it in only one code path, not two. >> I have not tried yet, but if we actually move the AllocateFile() and >> FreeFile() calls within tokenize_auth_file(), it looks like we may be >> able to get to a simpler logic without the need of the with_context() >> flavor (even no process_included_auth_file required)? That could make >> the interface easier to follow as a whole, while limiting the presence >> of AllocateFile() and FreeFile() to a single code path, impacting >> open_inc_file() that relies on what the patch uses for >> SecondaryAuthFile and IncludedAuthFile (we could attempt to use the >> same error message everywhere as well, as one could expect that >> expanded and included files have different names which is enough to >> guess which one is an inclusion and which one is a secondary). > > You meant tokenize_auth_file_internal? Yes possibly, in general I tried > to avoid changing too much the existing code to ease the patch acceptance, but > I agree it would make things simpler. I *guess* that my mind implied tokenize_auth_file() as of yesterday's. -- Michael signature.asc Description: PGP signature
Re: pg_basebackup's --gzip switch misbehaves
On Wed, Nov 02, 2022 at 09:42:12PM +0100, Daniel Gustafsson wrote: > I think that's a good idea, not everyone running tests will read the internals > documentation (or even know abou it even). How about the attached? Thanks for the patch. Perhaps this should be mentioned additionally in install-windows.sgml? I have not tested, but as long as these variables are configured with a "set" command in a command prompt, they would be passed down to the processes triggered by vcregress.pl (see for example TESTLOGDIR and TESTDATADIR). -- Michael signature.asc Description: PGP signature
Re: [PATCH] Add native windows on arm64 support
On Thu, Nov 03, 2022 at 11:06:46AM +, Niyas Sait wrote: > > I've gone ahead and marked it as "Committed", as that appears to have > happened > > back in August as 36389a060c. > > If for whatever reason that was the Wrong Thing To Do, please let me know. > > 36389a060c commit enables ASLR for windows but does not include other > changes required for Arm64. Yes, marking this patch as committed is incorrect. That's just me lagging behind for the review :) I have switched that back as "Needs review" for the moment. > I've attached a new version of the patch which excludes the already merged > ASLR changes and add > small changes to handle latest changes in the build scripts. Thanks. -- Michael signature.asc Description: PGP signature
Re: Incorrect include file order in guc-file.l
On Thu, Nov 03, 2022 at 12:40:19PM +0700, John Naylor wrote: > On Wed, Nov 2, 2022 at 1:01 PM Julien Rouhaud wrote: >> Agreed, it's apparently an oversight in dac048f71eb. +1 for the patch. > > I've pushed this, thanks! Thanks for the commit. I've wanted to get it done yesterday but life took over faster than that. Before committing the change, there is something I have noticed though: this header does not seem to be necessary at all and it looks that there is nothing in guc-file.l that needs it. Why did you add it in dac048f to begin with? -- Michael signature.asc Description: PGP signature
Add explicit casts in four places to simplehash.h
Hi hackers, While working on an extension, I found that simplehash.h is missing explicit casts in four places. Without these casts, compiling code including simplehash.h yields warnings if the code is compiled with -Wc++-compat. PostgreSQL seems to mostly prefer omitting the explicit casts, however there are many places where an explicit cast is actually used. Among many others, see e.g. bool.c: state = (BoolAggState *) MemoryContextAlloc(agg_context, sizeof(BoolAggState)); domains.c: my_extra = (DomainIOData *) MemoryContextAlloc(mcxt, sizeof(DomainIOData)); What about, while not being strictly necessary for PostgreSQL itself, also adding such casts to simplehash.h so that it can be used in code where -Wc++-compat is enabled? Attached is a small patch that adds the aforementioned casts. Thanks for your consideration! -- David Geier (ServiceNow) From d2665b4065227ffa6efd37ee912e4add082869cb Mon Sep 17 00:00:00 2001 From: David Geier Date: Thu, 3 Nov 2022 12:28:08 +0100 Subject: [PATCH] Add explicit casts to simplehash.h Without these casts compilation yields warnings when compiling the code with -Wc++-compat. --- src/include/lib/simplehash.h | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/include/lib/simplehash.h b/src/include/lib/simplehash.h index 329687c1a5..cdfae537a1 100644 --- a/src/include/lib/simplehash.h +++ b/src/include/lib/simplehash.h @@ -438,7 +438,7 @@ SH_CREATE(MemoryContext ctx, uint32 nelements, void *private_data) #ifdef SH_RAW_ALLOCATOR tb = SH_RAW_ALLOCATOR(sizeof(SH_TYPE)); #else - tb = MemoryContextAllocZero(ctx, sizeof(SH_TYPE)); + tb = (SH_TYPE *) MemoryContextAllocZero(ctx, sizeof(SH_TYPE)); tb->ctx = ctx; #endif tb->private_data = private_data; @@ -448,7 +448,7 @@ SH_CREATE(MemoryContext ctx, uint32 nelements, void *private_data) SH_COMPUTE_PARAMETERS(tb, size); - tb->data = SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * tb->size); + tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * tb->size); return tb; } @@ -493,7 +493,7 @@ SH_GROW(SH_TYPE * tb, uint64 newsize) /* compute parameters for new table */ SH_COMPUTE_PARAMETERS(tb, newsize); - tb->data = SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * tb->size); + tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * tb->size); newdata = tb->data; @@ -1059,7 +1059,7 @@ SH_STAT(SH_TYPE * tb) double fillfactor; uint32 i; - uint32 *collisions = palloc0(tb->size * sizeof(uint32)); + uint32 *collisions = (uint32 *) palloc0(tb->size * sizeof(uint32)); uint32 total_collisions = 0; uint32 max_collisions = 0; double avg_collisions; -- 2.34.1
Re: Pluggable toaster
Hi! Thank you very much for the feedback. Answering accordingly to questions/remarks order: >I'm in the process of testing and reviewing the v23 patchset. So far I >just wanted to report that there seems to be certain issues with the >formatting and/or trailing whitespaces in the patches: Accepted, will be done. >There are also some compiler warnings, please see the attachment. This too. There warnings showed up recently due to fresh changes in macros. >1.1. Could you please clarify what is the motivation behind such a >verbose syntax? Toaster is set for the table column. Each TOASTable column could have a different Toaster, so column option is the most obvious place to add it. >1.2. That's odd. TOAST should work for EXTENDED and MAIN storage >strategies as well. On top of that, why should custom TOASTers have >any knowledge of the default four-stage algorithm and the storage >strategies? If the storage strategy is actually ignored, it shouldn't >be used in the syntax. EXTENDED storage strategy means that TOASTed value is compressed before being TOASTed, so no knowledge of its internals could be of any use. EXTERNAL strategy means that value is being TOASTed in original form. Storage strategy is the thing internal to AM used, and TOAST mechanics is not meant to interfere with it. Again, STORAGE EXTERNAL explicitly shows that value will be stored out-of-line. >2. Although it's possible to implement some encryption in a TOASTer I >don't think the documentation should advertise this. It is a good example of what could the Toaster be responsible for, because we proposed moving compression into Toasters as a TOAST option - for example, being set while initializing the Toaster. >3.1. I believe we should rename this to something like `struct >ToastImpl`. The `Tsr` abbreviation only creates confusion, and this is >not a routine. It was done similar to Table AM Routine (please check Pluggable Storage API), along with some other decisions. >3.2. The names of the fields should be made consistent - e.g. if you >have update_toast and copy_toast then del_toast should be renamed to >delete_toast. >3.2. Additionally, in some parts of the path del_toast is used, while >in others - deltoast. Accepted, will be fixed in the next rebase. >4. The user documentation should have clear answers on the following questions: >4.1. What will happen if the user tries to delete a TOASTer while >still having data that was TOASTed using this TOASTer? Or this is not >supported and the TOASTers should exist in the database indefinitely >after creation? >4.2. Is it possible to delete and/or alter the default TOASTer? >4.3. Please make sure the previously discussed clarification regarding >"N TOASTers vs M TableAMs" and the validation function is explicitly >present. Thank you very much for checking the documentation package. These are very good comments, I'll include these topics in the next patchset. >5.1. The documentation should clarify how many times init() is called >- is it done once for the TOASTer, once per relation, etc. >5.2. Why there is no free() method? These too. About free() method - Toasters are not meant to be deleted, we mentioned this several times. They exist once they are created as long as the DB itself. Have I answered your question? >6. It's not clear for what reason update_toast() will be executed to >begin with. This should be clarified in the documentation. How does it >differ from copy_toast()? It is not clear because current TOAST mechanics does not have UPDATE functionality - it doesn't actually update TOASTed value, it marks this value "dead" and inserts a new one. This is the cause of TOAST tables bloating that is being complained about by many users. Update method is provided for implementation of UPDATE operation. >7. It should be explicitly stated when validate() is called and what >happens depending on the return value. Accepted, will correct this topic. >8. IMO this section does a very poor job in explaining what this is >for and when I may want to use this; what particular problem are we >addressing here? I already answered this question, maybe the answer was not very clear. This is just an extension entry point, because for some more advanced functionality existing pre-defined set of methods would be not enough, i.e. special Toasters for complex datatypes like JSONb, that have complex internal structure and may require additional ways to interact with it along toast/detoast/update/delete. Also, I'll check README and documentation for typos. On Thu, Nov 3, 2022 at 1:39 PM Aleksander Alekseev wrote: > Hi Nikita, > > > I'm going to submit a more detailed code review soon. > > As promised, here is my feedback. > > ``` > + Toaster could be assigned to toastable column with expression > + STORAGE external TOASTER > toaster_name > ``` > > 1.1. Could you please clarify what is the motivation behind such a > verbose syntax? > > ``` > +Please note that custom Toasters could
Re: confirmed_flush_lsn shows LSN of the data that has not yet been received by the logical subscriber.
On Mon, Sep 19, 2022 at 8:09 PM Ashutosh Sharma wrote: > > Thanks for the clarification. Attached is the patch with the changes. > Please have a look. > LGTM. I'll push this tomorrow unless there are any other comments/suggestions for this. -- With Regards, Amit Kapila.
Re: [PATCH] Add native windows on arm64 support
> I've gone ahead and marked it as "Committed", as that appears to have happened > back in August as 36389a060c. > If for whatever reason that was the Wrong Thing To Do, please let me know. 36389a060c commit enables ASLR for windows but does not include other changes required for Arm64. I've attached a new version of the patch which excludes the already merged ASLR changes and add small changes to handle latest changes in the build scripts. On Thu, 3 Nov 2022 at 08:38, Ian Lawrence Barwick wrote: > 2022年9月1日(木) 13:42 Michael Paquier : > > > > On Wed, Aug 31, 2022 at 10:22:06PM -0400, Tom Lane wrote: > > > Michael Paquier writes: > > > > The input object could also be reworked so as we would not have any > > > > ordering issues, say > "predeeppost". > > > > Changing only the path, you could use "/e/n2" instead of "node()", so > > > > as we'd always get the most internal member. > > > > > > Done that way. > > > > Cool, thanks. bowerbird looks happy after its first run. > > > > An argument in favor of backpatching is if one decides to build the > > code with MSVC and patch the scripts to enable ASLR. However, nobody > > has complained about that yet, so fine by me to leave this change only > > on HEAD for now. > > Apologies for the thread bump, but there is an entry for this thread > in the current CF > previously marked "Needs review": > > https://commitfest.postgresql.org/40/3561/ > > I've gone ahead and marked it as "Committed", as that appears to have > happened > back in August as 36389a060c. > > If for whatever reason that was the Wrong Thing To Do, please let me know. > > Regards > > Ian Barwick > -- Niyas Sait Linaro Limited v3-0001-Enable-postgres-native-build-for-windows-arm64-pl.patch Description: Binary data
Fix comments atop ReorderBufferAddInvalidations
The comments atop seem to indicate that we always accumulate invalidation messages in top-level transactions which is neither required nor match with the code. This is introduced in the commit c55040ccd0 and I have observed it while working on a fix for commit 16b1fe0037. -- With Regards, Amit Kapila. fix_comments_rb_invalidations_1.patch Description: Binary data
Re: Lockless queue of waiters in LWLock
Hi, hackers! > I'm planning to gather more detailed statistics on different > LWLockAcquire calls soon to understand prospects for further > optimizations. So, I've made more measurements. 1. Applied measuring patch 0001 to a patch with lockless queue optimization (v2) from [0] in this thread and run the same concurrent insert test described in [1] on 20 pgbench connections. The new results for ProcArray lwlock are as follows: exacq 45132 // Overall number of exclusive locks taken ex_attempt[0] 20755 // Exclusive locks taken immediately ex_attempt[1] 18800 // Exclusive locks taken after one waiting on semaphore ex_attempt[2] 5577// Exclusive locks taken after two or more waiting on semaphore shacq 494871// .. same stats for shared locks sh_attempt[0] 463211 // .. sh_attempt[1] 29767 // .. sh_attempt[2] 1893 // .. same stats for shared locks sh_wake_calls 31070 // Number of calls to wake up shared waiters sh_wakes 36190// Number of shared waiters woken up. GroupClearXid 55300 // Number of calls of ProcArrayGroupClearXid EndTransactionInternal: 236193 // Number of calls ProcArrayEndTransactionInternal 2. Applied measuring patch 0002 to a Andres Freund's patch v3 from [2] and run the same concurrent insert test described in [1] on 20 pgbench connections. The results for ProcArray lwlock are as follows: exacq 49300 // Overall number of exclusive locks taken ex_attempt1[0] 18353// Exclusive locks taken immediately by first call of LWLockAttemptLock in LWLockAcquire loop ex_attempt2[0] 18144. // Exclusive locks taken immediately by second call of LWLockAttemptLock in LWLockAcquire loop ex_attempt1[1] 9985 // Exclusive locks taken after one waiting on semaphore by first call of LWLockAttemptLock in LWLockAcquire loop ex_attempt2[1] 1838. // Exclusive locks taken after one waiting on semaphore by second call of LWLockAttemptLock in LWLockAcquire loop ex_attempt1[2] 823. // Exclusive locks taken after two or more waiting on semaphore by first call of LWLockAttemptLock in LWLockAcquire loop ex_attempt2[2] 157. // Exclusive locks taken after two or more waiting on semaphore by second call of LWLockAttemptLock in LWLockAcquire loop shacq 508131 // .. same stats for shared locks sh_attempt1[0] 469410 //.. sh_attempt2[0] 27858. //.. sh_attempt1[1] 10309. //.. sh_attempt2[1] 460. //.. sh_attempt1[2] 90. //.. sh_attempt2[2] 4. // .. same stats for shared locks dequeue self 48461 // Number of dequeue_self calls sh_wake_calls 27560 // Number of calls to wake up shared waiters sh_wakes 19408 // Number of shared waiters woken up. GroupClearXid 65021. // Number of calls of ProcArrayGroupClearXid EndTransactionInternal: 249003 // Number of calls ProcArrayEndTransactionInternal It seems that two calls in each look in Andres's (and master) code help evade semaphore-waiting loops that may be relatively expensive. The probable reason for this is that the small delay between these two calls is sometimes enough for concurrent takers to free spinlock for the queue modification. Could we get even more performance if we do three or more tries to take the lock in the queue? Will this degrade performance in some other cases? Or maybe there is another explanation for now small performance difference around 20 connections described in [0]? Thoughts? Regards, Pavel Borisov [0] https://www.postgresql.org/message-id/CALT9ZEF7q%2BSarz1MjrX-fM7OsoU7CK16%3DONoGCOkY3Efj%2BGrnw%40mail.gmail.com [1] https://www.postgresql.org/message-id/CALT9ZEEz%2B%3DNepc5eti6x531q64Z6%2BDxtP3h-h_8O5HDdtkJcPw%40mail.gmail.com [2] https://www.postgresql.org/message-id/20221031235114.ftjkife57zil7ryw%40awork3.anarazel.de 0001-Print-extended-lwlock_stats-and-proc_stats-on-CAS-re.patch Description: Binary data 0002-Print-extended-lwlock_stats-and-proc_stats.patch Description: Binary data
Re: Pluggable toaster
Hi Nikita,
> I'm going to submit a more detailed code review soon.
As promised, here is my feedback.
```
+ Toaster could be assigned to toastable column with expression
+ STORAGE external TOASTER
toaster_name
```
1.1. Could you please clarify what is the motivation behind such a
verbose syntax?
```
+Please note that custom Toasters could be assigned only to External
+storage type.
```
1.2. That's odd. TOAST should work for EXTENDED and MAIN storage
strategies as well. On top of that, why should custom TOASTers have
any knowledge of the default four-stage algorithm and the storage
strategies? If the storage strategy is actually ignored, it shouldn't
be used in the syntax.
```
+Toasters could use any storage, advanced compression, encryption, and
```
2. Although it's possible to implement some encryption in a TOASTer I
don't think the documentation should advertise this.
```
+typedef struct TsrRoutine
+{
+NodeTagtype;
+
+/* interface functions */
+toast_init init;
+toast_function toast;
+update_toast_function update_toast;
+copy_toast_function copy_toast;
+detoast_function detoast;
+del_toast_function deltoast;
+get_vtable_function get_vtable;
+toastervalidate_function toastervalidate;
+} TsrRoutine;
```
3.1. I believe we should rename this to something like `struct
ToastImpl`. The `Tsr` abbreviation only creates confusion, and this is
not a routine.
3.2. The names of the fields should be made consistent - e.g. if you
have update_toast and copy_toast then del_toast should be renamed to
delete_toast.
3.2. Additionally, in some parts of the path del_toast is used, while
in others - deltoast.
4. The user documentation should have clear answers on the following questions:
4.1. What will happen if the user tries to delete a TOASTer while
still having data that was TOASTed using this TOASTer? Or this is not
supported and the TOASTers should exist in the database indefinitely
after creation?
4.2. Is it possible to delete and/or alter the default TOASTer?
4.3. Please make sure the previously discussed clarification regarding
"N TOASTers vs M TableAMs" and the validation function is explicitly
present.
```
Toaster initialization. Initial TOAST table creation and other startup
preparations.
```
5.1. The documentation should clarify how many times init() is called
- is it done once for the TOASTer, once per relation, etc.
5.2. Why there is no free() method?
```
Updates TOASTed value. Returns new TOAST pointer.
```
6. It's not clear for what reason update_toast() will be executed to
begin with. This should be clarified in the documentation. How does it
differ from copy_toast()?
```
Validates Toaster for given data type, storage and compression options.
```
7. It should be explicitly stated when validate() is called and what
happens depending on the return value.
```
+Virtual Functions Table API Extension
```
8. IMO this section does a very poor job in explaining what this is
for and when I may want to use this; what particular problem are we
addressing here?
9. There are typos in the comments and the documentation,
s/Vaitual/Virtual/, s/vurtual/virtual/ etc. Also there are missing
articles. Please run your patches through a spellchecker.
I suggest we address this piece of feedback before proceeding further.
--
Best regards,
Aleksander Alekseev
Re: Allow single table VACUUM in transaction block
On Tue, 1 Nov 2022 at 23:56, Simon Riggs wrote: > > I haven't checked the rest of the patch, but +1 for allowing VACUUM FULL > > within a user txn. > > My intention was to prevent that. I am certainly quite uneasy about > changing anything related to CLUSTER/VF, since they are old, complex > and bug prone. > > So for now, I will block VF, as was my original intent. > > I will be guided by what others think... so you may yet get your wish. > > > > Maybe the error message needs to be qualified "...when multiple > > relations are specified". > > > > ERROR: VACUUM cannot run inside a transaction block > > Hmm, that is standard wording based on the statement type, but I can > set a CONTEXT message also. Will update accordingly. > > Thanks for your input. New version attached, as described. Other review comments and alternate opinions welcome. -- Simon Riggshttp://www.EnterpriseDB.com/ single_table_vacuum.v2.patch Description: Binary data
Re: Commit fest 2022-11
2022年11月3日(木) 11:33 Ian Lawrence Barwick : > > 2022年11月2日(水) 19:10 Greg Stark : > > > > On Tue, 1 Nov 2022 at 06:56, Michael Paquier wrote: > > > > > Two people showing up to help is really great, thanks! I'll be around > > > as well this month, so I'll do my share of patches, as usual. > > > > Fwiw I can help as well -- starting next week. I can't do much this week > > though. > > > > I would suggest starting with the cfbot to mark anything that isn't > > applying cleanly and passing tests (and looking for more than design > > feedback) as Waiting on Author and reminding the author that it's > > commitfest time and a good time to bring the patch into a clean state. > > Sounds like a plan; I'll make a start on that today/tomorrow as I have > some time. Ploughing through the list, initially those where the patches don't apply. I am wondering what the best thing to do with cases like this is: https://commitfest.postgresql.org/40/3977/ where there were multiple patches in the original post, and some but not all were applied - so those ones are now failing to apply in the cfbot. Should we request the author to update the thread with those patches which are still pending? Regards Ian Barwick
Re: Adding CommandID to heap xlog records
2022年9月30日(金) 1:04 Matthias van de Meent : > > On Wed, 28 Sept 2022 at 19:40, Bruce Momjian wrote: > > > > On Thu, Sep 22, 2022 at 11:12:32PM +0200, Matthias van de Meent wrote: > > > On Thu, 8 Sept 2022 at 23:24, Tom Lane wrote: > > > > > > > > Matthias van de Meent writes: > > > > > Please find attached a patch that adds the CommandId of the inserting > > > > > transaction to heap (batch)insert, update and delete records. It is > > > > > based on the changes we made in the fork we maintain for Neon. > > > > > > > > This seems like a very significant cost increment with returns > > > > to only a minuscule number of users. We certainly cannot consider > > > > it unless you provide some evidence that that impression is wrong. > > > > > > Attached a proposed set of patches to reduce overhead of the inital patch. > > > > This might be obvious to some, but the patch got a lot larger. :-( > > Sorry for that, but updating the field from which the redo manager > should pull its information does indeed touch a lot of files because > most users of xl_info are only interested in the 4 bits reserved for > the redo-manager. Most of 0001 is therefore updates to point code to > the new field in XLogRecord, and renaming the variables and arguments > from info to rminfo. > > [tangent] With that refactoring, I also clean up a lot of code that > was using a wrong macro/constant for rmgr flags; `info & > ~XLR_INFO_MASK` may have the same value as `info & > XLR_RMGR_INFO_MASK`, but that's only guaranteed by the documentation; > and would require the same significant rework if new bits were > assigned to non-XLR_INFO_MASK and non-XLR_RMGR_INFO_MASK. [/tangent] > > 0002 grew a bit as well, but not to a degree that I think is worrying > or otherwise impossible to review. Hi This entry was marked as "Needs review" in the CommitFest app but cfbot reports the patch no longer applies. We've marked it as "Waiting on Author". As CommitFest 2022-11 is currently underway, this would be an excellent time update the patch. Once you think the patchset is ready for review again, you (or any interested party) can move the patch entry forward by visiting https://commitfest.postgresql.org/40/3882/ and changing the status to "Needs review". Thanks Ian Barwick
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
2022年11月3日(木) 17:15 Maxim Orlov : > > Hi! > >> >> This entry was marked "Ready for committer" in the CommitFest app but cfbot >> reports the patch no longer applies. > > Thanks for the reminder. I think, this should work. Thanks for the quick update, cfbot reports no issues. I've set the CF entry back to "Ready for Committer", Regards Ian Barwick
Re: Check SubPlan clause for nonnullable rels/Vars
On Thu, Nov 3, 2022 at 4:26 AM Tom Lane wrote: > * I don't believe you can prove anything from an ALL_SUBLINK SubPlan, > because it will return true if the sub-query returns zero rows, no > matter what the testexpr is. (Maybe if you could prove the sub-query > does return a row, but I doubt it's worth going there.) Thanks for pointing this out. You're right. I didn't consider the case that the subplan produces zero rows. In this case ALL_SUBLINK will always return true, and ANY_SUBLINK will always return false. That makes ALL_SUBLINK not strict, and ANY_SUBLINK can be strict only at top level. * You need to explicitly check the subLinkType; as written this'll > consider EXPR_SUBLINK and so on. I'm not really on board with > assuming that nothing bad will happen with sublink types other than > the ones the code is expecting. > Yes, I need to check for ANY_SUBLINK and ROWCOMPARE_SUBLINK here. The testexpr is only meaningful for ALL/ANY/ROWCOMPARE, and ALL_SUBLINK has been proven not strict. * It's not apparent to me that it's okay to pass down "top_level" > rather than "false". Maybe it's all right, but it could do with > a comment. The 'top_level' param is one point that I'm not very confident about. I've added comments in the v2 patch. Thanks for reviewing this patch! Thanks Richard v2-0001-Check-SubPlan-clause-for-nonnullable-rels-Vars.patch Description: Binary data
Re: Prefetch the next tuple's memory during seqscans
On Tue, Nov 1, 2022 at 5:17 AM David Rowley wrote:
>
> My test is to run 16 queries changing the WHERE clause each time to
> have WHERE a = 0, then WHERE a2 = 0 ... WHERE a16 = 0. I wanted to
> know if prefetching only the first cache line of the tuple would be
> less useful when we require evaluation of say, the "a16" column vs the
> "a" column.
I tried a similar test, but with text fields of random length, and there is
improvement here:
Intel laptop, turbo boost off
shared_buffers = '4GB'
huge_pages = 'on'
max_parallel_workers_per_gather = '0'
create table text8 as
select
repeat('X', int4(random() * 20)) a1,
repeat('X', int4(random() * 20)) a2,
repeat('X', int4(random() * 20)) a3,
repeat('X', int4(random() * 20)) a4,
repeat('X', int4(random() * 20)) a5,
repeat('X', int4(random() * 20)) a6,
repeat('X', int4(random() * 20)) a7,
repeat('X', int4(random() * 20)) a8
from generate_series(1,1000) a;
vacuum freeze text8;
psql -c "select pg_prewarm('text8')" && \
for i in a1 a2 a3 a4 a5 a6 a7 a8;
do
echo Testing $i
echo "select * from text8 where $i = 'ZZZ';" > bench.sql
pgbench -f bench.sql -M prepared -n -T 10 postgres | grep latency
done
Master:
Testing a1
latency average = 980.595 ms
Testing a2
latency average = 1045.081 ms
Testing a3
latency average = 1107.736 ms
Testing a4
latency average = 1162.188 ms
Testing a5
latency average = 1213.985 ms
Testing a6
latency average = 1272.156 ms
Testing a7
latency average = 1318.281 ms
Testing a8
latency average = 1363.359 ms
Patch 0001+0003:
Testing a1
latency average = 812.548 ms
Testing a2
latency average = 897.303 ms
Testing a3
latency average = 955.997 ms
Testing a4
latency average = 1023.497 ms
Testing a5
latency average = 1088.494 ms
Testing a6
latency average = 1149.418 ms
Testing a7
latency average = 1213.134 ms
Testing a8
latency average = 1282.760 ms
--
John Naylor
EDB: http://www.enterprisedb.com
Re: Segfault on logical replication to partitioned table with foreign children
On Wed, 2022-11-02 at 12:37 -0400, Tom Lane wrote:
> Since we're getting pretty close to the next set of back-branch
> releases,
> I went ahead and pushed a minimal fix along the lines suggested by
> Shi Yu.
> (I realized that there's a second ExecFindPartition call in worker.c
> that
> also needs a check.) We still can at leisure think about refactoring
> CheckSubscriptionRelkind to avoid unnecessary lookups. I think that
> is something we should do only in HEAD; it'll just be a marginal
> savings,
> not worth the risks of API changes in stable branches. The other
> loose
> end is whether to worry about a regression test case. I'm inclined
> not
> to bother. The only thing that isn't getting exercised is the actual
> ereport, which probably isn't in great need of routine testing.
>
> regards, tom lane
I agree that early checks limit some of the functionality that was
available before, so I guess the only way to preserve it is to do only
the last-minute checks after routing, fair enough. As for the
refactoring of the premature lookup, I have done some work on that in
the previous patch, I think we can just use it. Attached a separate
patch with the refactoring.
From 004c63a8eba777be739f062cdc9b7ddcf2eac253 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev
Date: Thu, 3 Nov 2022 11:39:24 +0400
Subject: [PATCH] Delay namespace and relname lookups until error
---
src/backend/commands/subscriptioncmds.c| 12
src/backend/executor/execReplication.c | 13 ++---
src/backend/replication/logical/relation.c | 2 +-
src/backend/replication/logical/worker.c | 8 ++--
src/include/executor/executor.h| 2 +-
5 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index f0cec2ad5e..0c3ad698eb 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -700,12 +700,14 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
{
RangeVar *rv = (RangeVar *) lfirst(lc);
Oid relid;
+Relation sub_rel;
relid = RangeVarGetRelid(rv, AccessShareLock, false);
+sub_rel = RelationIdGetRelation(relid);
/* Check for supported relkind. */
-CheckSubscriptionRelkind(get_rel_relkind(relid),
- rv->schemaname, rv->relname);
+CheckSubscriptionRelkind(sub_rel, rv->schemaname, rv->relname);
+RelationClose(sub_rel);
AddSubscriptionRelState(subid, relid, table_state,
InvalidXLogRecPtr);
@@ -861,12 +863,14 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data,
{
RangeVar *rv = (RangeVar *) lfirst(lc);
Oid relid;
+ Relation sub_rel;
relid = RangeVarGetRelid(rv, AccessShareLock, false);
+ sub_rel = RelationIdGetRelation(relid);
/* Check for supported relkind. */
- CheckSubscriptionRelkind(get_rel_relkind(relid),
- rv->schemaname, rv->relname);
+ CheckSubscriptionRelkind(sub_rel, rv->schemaname, rv->relname);
+ RelationClose(sub_rel);
pubrel_local_oids[off++] = relid;
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index 6014f2e248..a0a2d326db 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -649,16 +649,23 @@ CheckCmdReplicaIdentity(Relation rel, CmdType cmd)
/*
* Check if we support writing into specific relkind.
*
- * The nspname and relname are only needed for error reporting.
+ * If nspname and relname are not NULL, they are used for error reporting, otherwise these values are fetched from relcache.
*/
void
-CheckSubscriptionRelkind(char relkind, const char *nspname,
- const char *relname)
+CheckSubscriptionRelkind(Relation rel, const char *nspname, const char *relname)
{
+ char relkind = rel->rd_rel->relkind;
+
if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
+ {
+ if (relname == NULL)
+ relname = RelationGetRelationName(rel);
+ if (nspname == NULL)
+ nspname = get_namespace_name(RelationGetNamespace(rel));
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot use relation \"%s.%s\" as logical replication target",
nspname, relname),
errdetail_relkind_not_supported(relkind)));
+ }
}
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e989047681..e98031e272 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -393,7 +393,7 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
entry->localreloid = relid;
/* Check for supported relkind. */
- CheckSubscriptionRelkind(entry->localrel->rd_rel->relkind,
+ CheckSubscriptionRelkind(entry->localrel,
remoterel->nspname, remoterel->relname);
/*
diff --git a/src/backend/replication/logical/worker.c
Re: [PATCH] Add native windows on arm64 support
2022年9月1日(木) 13:42 Michael Paquier : > > On Wed, Aug 31, 2022 at 10:22:06PM -0400, Tom Lane wrote: > > Michael Paquier writes: > > > The input object could also be reworked so as we would not have any > > > ordering issues, say "predeeppost". > > > Changing only the path, you could use "/e/n2" instead of "node()", so > > > as we'd always get the most internal member. > > > > Done that way. > > Cool, thanks. bowerbird looks happy after its first run. > > An argument in favor of backpatching is if one decides to build the > code with MSVC and patch the scripts to enable ASLR. However, nobody > has complained about that yet, so fine by me to leave this change only > on HEAD for now. Apologies for the thread bump, but there is an entry for this thread in the current CF previously marked "Needs review": https://commitfest.postgresql.org/40/3561/ I've gone ahead and marked it as "Committed", as that appears to have happened back in August as 36389a060c. If for whatever reason that was the Wrong Thing To Do, please let me know. Regards Ian Barwick
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Thu, Nov 3, 2022 at 1:46 PM Maxim Orlov wrote: > > Hi! > >> >> This entry was marked "Ready for committer" in the CommitFest app but cfbot >> reports the patch no longer applies. > > Thanks for the reminder. I think, this should work. Have we measured the WAL overhead because of this patch set? maybe these particular patches will not impact but IIUC this is ground work for making xid 64 bit. So each XLOG record size will increase at least by 4 bytes because the XLogRecord contains the xid. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: Add index scan progress to pg_stat_progress_vacuum
2022年11月3日(木) 1:52 Imseih (AWS), Sami : > > Attached is v13-0001--Show-progress-for-index-vacuums.patch which addresses > the latest comments. The main changes are: > > 1/ Call the parallel_vacuum_progress_report inside the AMs rather than > vacuum_delay_point. > > 2/ A Boolean when set to True in vacuumparallel.c will be used to determine > if calling > parallel_vacuum_progress_report is necessary. > > 3/ Removed global varilable from vacuumparallel.c > > 4/ Went back to calling parallel_vacuum_progress_report inside > WaitForParallelWorkersToFinish to cover the case when a > leader is waiting for parallel workers to finish. > > 5/ I did not see a need to only report progress after 1GB as it's a fairly > cheap call to update > progress. > > 6/ v1-0001-Function-to-return-currently-vacuumed-or-cleaned-ind.patch is a > separate patch > for exposing the index relid being vacuumed by a backend. This entry was marked "Needs review" in the CommitFest app but cfbot reports the patch [1] no longer applies. [1] this patch: v1-0001-Function-to-return-currently-vacuumed-or-cleaned-ind.patch We've marked it as "Waiting on Author". As CommitFest 2022-11 is currently underway, this would be an excellent time update the patch. Once you think the patchset is ready for review again, you (or any interested party) can move the patch entry forward by visiting https://commitfest.postgresql.org/40/3617/ and changing the status to "Needs review". Thanks Ian Barwick
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
2022年10月10日(月) 18:16 Aleksander Alekseev : > > Hi hackers, > > > Sorry about that. > > No problem. > > > Thanks, Simon. Not 100% sure who exactly is invited to vote, but just > > in case here is +1 from me. These patches were "Ready for Committer" > > for several months now and are unlikely to get any better. So I > > suggest we merge them. > > Here is the rebased patchset number 44. Hi This entry was marked "Ready for committer" in the CommitFest app but cfbot reports the patch no longer applies. We've marked it as "Waiting on Author". As CommitFest 2022-11 is currently underway, this would be an excellent time update the patch. Once you think the patchset is ready for review again, you (or any interested party) can move the patch entry forward by visiting https://commitfest.postgresql.org/40/3489/ and changing the status to "Ready for committer". Thanks Ian Barwick
Re: Adding doubly linked list type which stores the number of items in the list
On Wed, Nov 2, 2022 at 2:23 PM Aleksander Alekseev wrote: > > Hi David, > > > Pushed. Thank you both for reviewing this. > > Thanks for applying the patch. > > >> Should we consider const'ifying the arguments of the dlist_*/dclist_* > >> functions that don't change the arguments? > >> [...] > > You're right, both of them must be discussed separately. > > I would like to piggyback on this thread to propose the const'ifying > patch, if that's OK. Here it is. Thanks for the patch. IMO, this can be discussed in a separate thread to get more thoughts from the hackers. BTW, there's proclist_* data structure which might need the similar const'ifying. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Support logical replication of DDLs
Here are some more review comments for the v32-0001 file ddl_deparse.c
(This is a WIP since it is such a large file)
==
1. General - calling VA with 0 args
There are some calls to new_objtree_VA() where 0 extra args are passed.
e.g. See in deparse_AlterFunction
* alterFunc = new_objtree_VA("ALTER FUNCTION", 0);
* ObjTree*tmpobj = new_objtree_VA("%{type}T", 0);
* tmpobj = new_objtree_VA(intVal(defel->arg) ? "RETURNS NULL ON NULL
INPUT" : "CALLED ON NULL INPUT", 0);
* tmpobj = new_objtree_VA(intVal(defel->arg) ? "SECURITY DEFINER" :
"SECURITY INVOKER", 0);
* tmpobj = new_objtree_VA(intVal(defel->arg) ? "LEAKPROOF" : "NOT
LEAKPROOF", 0);
* etc.
Shouldn't all those just be calling the new_objtree() function instead
of new_objtree_VA()?
Probably there are more than just those cited - please search for others.
~~~
2. General - when to call append_xxx functions?
I did not always understand what seems like an arbitrary choice of
function calls to append_xxx.
e.g. Function deparse_AlterObjectSchemaStmt() does:
+ append_string_object(alterStmt, "%{identity}s", ident);
+
+ append_string_object(alterStmt, "SET SCHEMA %{newschema}I", newschema);
Why doesn't the above just use new_objtree_VA instead -- it seemed to
me like the _VA function is underused in some places. Maybe search all
the append_xxx usage - I suspect many of those can in fact be combined
to use new_objtree_VA().
~~~
3. General - extract object names from formats
IIUC the calls to append_XXX__object will call deeper to
append_object_to_format_string(), which has a main side-effect loop to
extract the "name" part out of the sub_fmt string. But this logic all
seems inefficient and unnecessary to me. I think in most (all?) cases
the caller already knows what the object name should be, so instead of
making code work to figure it out again, it can just be passed in the
same way the _VA() function passes the known object name.
There are many cases of this:
e.g.
BEFORE
append_string_object(alterop, "(%{left_type}s", "NONE");
AFTER - just change the signature to pass the known object name
append_string_object(alterop, "(%{left_type}s", "left_type", "NONE");
~~~
4. General - fwd declares
static void append_array_object(ObjTree *tree, char *name, List *array);
static void append_bool_object(ObjTree *tree, char *name, bool value);
static void append_float_object(ObjTree *tree, char *name, float8 value);
static void append_null_object(ObjTree *tree, char *name);
static void append_object_object(ObjTree *tree, char *name, ObjTree *value);
static char *append_object_to_format_string(ObjTree *tree, char *sub_fmt);
static void append_premade_object(ObjTree *tree, ObjElem *elem);
static void append_string_object(ObjTree *tree, char *name, char *value);
I think those signatures are misleading. AFAIK seems what is called
the 'name' param above is often a 'sub_fmt' param in the
implementation.
~~~
5. General - inconsistent append_array object calls.
Sometimes enclosing brackets are included as part of the format string
to be appended and other times they are appended separately. IIUC
there is no difference but IMO the code should always be consistent to
avoid it being confusing.
e.g.1 (brackets in fmt)
append_array_object(tmpobj, "(%{rettypes:, }s)", rettypes);
e.g.2 (brackets appended separately)
+ append_format_string(tmpobj, "(");
+ append_array_object(tmpobj, "%{argtypes:, }T", arglist);
+ append_format_string(tmpobj, ")");
~~~
6. General - missing space before '('
I noticed a number of comment where there is a space missing before a '('.
Here are some examples:
- * An element of an object tree(ObjTree).
- * typmod at the middle of name(e.g. TIME(6) with time zone ). We cannot
- * Sequence for IDENTITY COLUMN output separately(via CREATE TABLE or
- * Sequence for IDENTITY COLUMN output separately(via CREATE TABLE or
Search all the patch-code to find others and add missing spaces.
~~~
7. General - Verbose syntax comments
Some (but not all) of the deparse_XXX functions have a comment
describing the verbose syntax.
e.g.
/*
* Verbose syntax
*
* CREATE %{default}s CONVERSION %{identity}D FOR %{source}L TO %{dest}L
* FROM %{function}D
*/
These are helpful for understanding the logic of the function, so IMO
similar comments should be written for *all* of the deparse_xxx
function.
And maybe a more appropriate place to put these comments is in the
function header comment.
==
8. new_objtree_VA
+ /*
+ * For all other param types there must be a value in the varargs.
+ * Fetch it and add the fully formed subobject into the main object.
+ */
+ switch (type)
What does the comment mean when it says - for all "other" param types?
~~~
9. objtree_to_jsonb_element
+ ListCell *cell;
+ JsonbValue val;
The 'cell' is only for the ObjTypeArray so consider declaring it for
that case only.
~~~
10. obtainConstraints
+ else
+ {
+ Assert(OidIsValid(domainId));
Looks like the above Assert is
Re: Improve description of XLOG_RUNNING_XACTS
2022年11月2日(水) 15:24 Amit Kapila : > > On Tue, Nov 1, 2022 at 4:33 PM Amit Kapila wrote: > > > > On Tue, Nov 1, 2022 at 12:53 PM Michael Paquier wrote: > > > > > > On Mon, Oct 17, 2022 at 02:53:57PM +0900, Michael Paquier wrote: > > > > No objections from here if you want to go ahead with v3 and print the > > > > full set of subxids on top of the information about these > > > > overflowing. > > > > > > While browsing the CF entries, this was still listed. Amit, any > > > updates? > > > > > > > I am planning to take care of this entry sometime this week. > > > > Pushed. Marked as committed in the CF app, many thanks for closing this one out. Regards Ian Barwick
