Re: pgbench logging broken by time logic changes
On Sun, Jun 20, 2021 at 4:52 PM Thomas Munro wrote: > On Sun, Jun 20, 2021 at 3:18 PM Alvaro Herrera > wrote: > > On 2021-Jun-19, Thomas Munro wrote: > > > Thanks for looking so far. It's the weekend here and I need to > > > unplug, but I'll test these changes and if all looks good push on > > > Monday. > > > > Surely not the same day as the beta stamp... > > Because of timezones, that'll be Sunday in the Americas. Still too close? Upon reflection, that amounts to the same thing really, so yeah, scratch that plan. I'll defer until after that (and then I'll be leaning more towards the revert option).
Re: unnesting multirange data types
On Sat, Jun 19, 2021 at 10:05:09PM -0400, Tom Lane wrote: > Alexander Korotkov writes: > > I also don't feel comfortable hurrying with unnest part to beta2. > > According to the open items wiki page, there should be beta3. Does > > unnest part have a chance for beta3? > > Hm. I'd prefer to avoid another forced initdb after beta2. On the > other hand, it's entirely likely that there will be some other thing > that forces that; in which case there'd be no reason not to push in > the unnest feature as well. > > I'd say let's sit on the unnest code for a little bit and see what > happens. I think $SUBJECT can't simultaneously offer too little to justify its own catversion bump and also offer enough to bypass feature freeze. If multirange is good without $SUBJECT, then $SUBJECT should wait for v15. Otherwise, the matter of the catversion bump should not delay commit of $SUBJECT.
Re: pgbench logging broken by time logic changes
Upon reflection, that amounts to the same thing really, so yeah, scratch that plan. I'll defer until after that (and then I'll be leaning more towards the revert option). Sigh. I do not understand anything about the decision processus. If you do revert, please consider NOT reverting the tps computation changes intermixed in the patch. -- Fabien.
Re: seawasp failing, maybe in glibc allocator
Hi, On 2021-06-19 17:07:51 +1200, Thomas Munro wrote: > On Sat, May 22, 2021 at 12:25 PM Andres Freund wrote: > > On 2021-05-21 15:57:01 -0700, Andres Freund wrote: > > > I found the LLVM commit to blame > > > (c8fc5e3ba942057d6c4cdcd1faeae69a28e7b671). > > > Contacting the author and reading the change to see if I can spit the > > > issue myself. > > > > Hrmpf. It's a silent API breakage. The author intended to email us about > > it, but apparently forgot. One now needs to increment a string-pool > > refcount. The reason that didn't trigger a reliable crash is that > > there's a path where the refcount of string-pool entries aren't asserted > > to be above before decrementing the refcount... And that there > > practically never are references to the pool entries after use. > > > > Continuing to discusss whether there's a better way to deal with this. > > Any news? Nothing really. I'd discussed it a bit with the relevant LLVM maintainer, but we didn't come to a real resolution. He apologized for not giving a proper heads up - he'd planned to send out an email, but somehow lost track of that. Given that any potential solution would be also end up being a versioned ifdef, I think adding something like what you suggest here is the least unreasonable solution. > FWIW this change appears to fix the problem for my system (LLVM 13 > build from a couple of days ago). No more weird results, valgrind > errors gone. I ran the leak checker to see if I now had the opposite > problem, and although there are various leaks reported, I didn't see > obvious intern pool related stacks. > > diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c > index 71029a39a9..7b09e520f5 100644 > --- a/src/backend/jit/llvm/llvmjit.c > +++ b/src/backend/jit/llvm/llvmjit.c > @@ -1116,6 +1116,11 @@ > llvm_resolve_symbols(LLVMOrcDefinitionGeneratorRef GeneratorObj, void > *Ctx, > if (error != LLVMErrorSuccess) > LLVMOrcDisposeMaterializationUnit(mu); > > +#if LLVM_VERSION_MAJOR > 12 > + for (int i = 0; i < LookupSetSize; i++) > + LLVMOrcRetainSymbolStringPoolEntry(symbols[i].Name); > +#endif > + > pfree(symbols); I think this should be part of the earlier loop? Once LLVMOrcAbsoluteSymbols() is called that owns the reference, so there doesn't seem to be a reason to increase the refcount only later? Greetings, Andres Freund
Re: seawasp failing, maybe in glibc allocator
Hi, On 2021-06-19 10:12:03 -0400, Tom Lane wrote: > Is a compile-time conditional really going to be reliable? See nearby > arguments about compile-time vs run-time checks for libpq features. > It's not clear to me how tightly LLVM binds its headers and running > code. It should be fine (and if not we have plenty other places it'd be problematic). LLVM embeds the version between user of llvm and the library version in some symbol, so if there's a sufficient mismatch it'll cause link time issues. Of course that only works for major versions, but that shouldn't be an issue here. Greetings, Andres Freund
RE: locking [user] catalog tables vs 2pc vs logical rep
On Sunday, June 20, 2021 3:23 PM Amit Kapila wrote: > On Sun, Jun 20, 2021 at 9:28 AM osumi.takami...@fujitsu.com > wrote: > > * doc/src/sgml/logicaldecoding.sgml ... > > > > Now we have the four paren supplementary descriptions, not to make > > users miss any other [user] catalog tables. > > Because of this, the built output html gives me some redundant > > impression, for that parts. Accordingly, couldn't we move them to > > outside of the itemizedlist section in a simple manner ? > > > > For example, to insert a sentence below the list, after removing the > > paren descriptions in the listitem, which says "Note that those > > commands that can cause deadlock apply to not only explicitly > > indicated system catalog tables above but also any other [user] catalog > > table." > > Sounds reasonable to me. /but also any other/but also to any other/, to > seems to be missing in the above line. Kindly send an update patch. Excuse me, I don't understand the second sentence. I wrote "but also" clause in my example. Also, attached the patch for the change to the HEAD. Best Regards, Takamichi Osumi v3-0001-Doc-Update-caveats-in-synchronous-logical-replica.patch Description: v3-0001-Doc-Update-caveats-in-synchronous-logical-replica.patch
PXGS vs TAP tests
The recipe for running TAP tests in src/Makefile.global doesn't work for the PGXS case. If you try it you get something like this: andrew@emma:tests $ make PG_CONFIG=../inst.head.5701/bin/pg_config installcheck rm -rf '/home/andrew/pgl/tests'/tmp_check /usr/bin/mkdir -p '/home/andrew/pgl/tests'/tmp_check cd ./ && TESTDIR='/home/andrew/pgl/tests' PATH="/home/andrew/pgl/inst.head.5701/bin:$PATH" PGPORT='65701' \ top_builddir='/home/andrew/pgl/tests//home/andrew/pgl/inst.head.5701/lib/postgresql/pgxs/src/makefiles/../..' \ PG_REGRESS='/home/andrew/pgl/tests//home/andrew/pgl/inst.head.5701/lib/postgresql/pgxs/src/makefiles/../../src/test/regress/pg_regress' \ REGRESS_SHLIB='/src/test/regress/regress.so' \ /usr/bin/prove -I /home/andrew/pgl/inst.head.5701/lib/postgresql/pgxs/src/makefiles/../../src/test/perl/ -I ./ t/*.pl Notice those bogus settings for top_builddir, PG_REGRESS and REGRESS_SHLIB. The attached patch fixes this bug. With it you can get by with a Makefile as simple as this for running TAP tests under PGXS: TAP_TESTS = 1 PG_CONFIG = pg_config PGXS := $(shell $(PG_CONFIG) --pgxs) include $(PGXS) I removed the REGRESS_SHLIB setting altogether for the PGXS case - it's not clear to me why we need it in a TAP test recipe at all. Certainly it's not installed anywhere in a standard install so it seems entirely bogus for the PGXS case. This seems like a bug fix that should be patched all the way back, although I haven't yet investigated the back branches. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com diff --git a/src/Makefile.global.in b/src/Makefile.global.in index 8f05840821..ab74535918 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -444,11 +444,19 @@ with_temp_install = \ ifeq ($(enable_tap_tests),yes) +ifndef PGXS define prove_installcheck rm -rf '$(CURDIR)'/tmp_check $(MKDIR_P) '$(CURDIR)'/tmp_check cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' REGRESS_SHLIB='$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl) endef +else # PGXS case +define prove_installcheck +rm -rf '$(CURDIR)'/tmp_check +$(MKDIR_P) '$(CURDIR)'/tmp_check +cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(top_builddir)' PG_REGRESS='$(top_builddir)/src/test/regress/pg_regress' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl) +endef +endif # PGXS define prove_check rm -rf '$(CURDIR)'/tmp_check
Re: Speed up transaction completion faster after many relations are accessed in a transaction
On Wed, 14 Aug 2019 at 19:25, David Rowley wrote: > For now, I'm out of ideas. If anyone else feels like suggesting > something of picking this up, feel free. This is a pretty old thread, so we might need a recap: # Recap Basically LockReleaseAll() becomes slow after a large number of locks have all been held at once by a backend. The problem is that the LockMethodLocalHash dynahash table must grow to store all the locks and when later transactions only take a few locks, LockReleaseAll() is slow due to hash_seq_search() having to skip the sparsely filled buckets in the bloated hash table. The following things were tried on this thread. Each one failed: 1) Use a dlist in LOCALLOCK to track the next and prev LOCALLOCK. Simply loop over the dlist in LockReleaseAll(). 2) Try dropping and recreating the dynahash table if it becomes bloated using some heuristics to decide what "bloated" means and if recreating is worthwhile. #1 failed due to concerns with increasing the size of LOCALLOCK to store the dlist pointers. Performance regressions were seen too. Possibly due to size increase or additional overhead from pushing onto the dlist. #2 failed because it was difficult to agree on what the heuristics would be and we had no efficient way to determine the maximum number of locks that a given transaction held at any one time. We only know how many were still held at LockReleaseAll(). There were also some suggestions to fix dynahash's hash_seq_search() slowness, and also a suggestion to try using simplehash.h instead of dynahash.c. Unfortunately simplehash.h would suffer the same issues as it too would have to skip over empty buckets in a sparsely populated hash table. I'd like to revive this effort as I have a new idea on how to solve the problem. # Background Over in [1] I'm trying to improve the performance of smgropen() during recovery. The slowness there comes from the dynahash table lookups to find the correct SMgrRelation. Over there I proposed to use simplehash instead of dynahash because it's quite a good bit faster and far lessens the hash lookup overhead during recovery. One problem on that thread is that relcache keeps a pointer into the SMgrRelation (RelationData.rd_smgr) and because simplehash moves things around during inserts and deletes, then we can't have anything point to simplehash entries, they're unstable. I fixed that over on the other thread by having the simplehash entry point to a palloced SMgrRelationData... My problem is, I don't really like that idea as it means we need to palloc() pfree() lots of little chunks of memory. To fix the above, I really think we need a version of simplehash that has stable pointers. Providing that implementation is faster than dynahash, then it will help solve the smgropen() slowness during recovery. # A new hashtable implementation I ended up thinking of this thread again because the implementation of the stable pointer hash that I ended up writing for [1] happens to be lightning fast for hash table sequential scans, even if the table has become bloated. The reason the seq scans are so fast is that the implementation loops over the data arrays, which are tightly packed and store the actual data rather than pointers to the data. The code does not need to loop over the bucket array for this at all, so how large that has become is irrelevant to hash table seq scan performance. The patch stores elements in "segments" which is set to some power of 2 value. When we run out of space to store new items in a segment, we just allocate another segment. When we remove items from the table, new items reuse the first unused item in the first segment with free space. This helps to keep the used elements tightly packed. A segment keeps a bitmap of used items so that means scanning all used items is very fast. If you flip the bits in the used_item bitmap, then you get a free list for that segment, so it's also very fast to find a free element when inserting a new item into the table. I've called the hash table implementation "generichash". It uses the same preprocessor tricks as simplehash.h does and borrows the same linear probing code that's used in simplehash. The bucket array just stores the hash value and a uint32 index into the segment item that stores the data. Since segments store a power of 2 items, we can easily address both the segment number and the item within the segment from the single uint32 index value. The 'index' field just stores a special value when the bucket is empty. No need to add another field for that. This means the bucket type is just 8 bytes wide. One thing I will mention about the new hash table implementation is that GH_ITEMS_PER_SEGMENT is, by default, set to 256. This means that's the minimum size for the table. I could drop this downto 64, but that's still quite a bit more than the default size of the dynahash table of 16. I think 16 is a bit on the low side and that it might be worth making this 64 anyway. I
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
В письме от пятница, 4 июня 2021 г. 3:19:09 MSK пользователь Jeff Davis написал: > > Moving reloptions to AM code is the goal I am slowly moving to. I've > > started > > some time ago with big patch > > https://commitfest.postgresql.org/14/992/ and > > have been told to split it into smaller parts. So I did, and this > > patch is > > last step that cleans options related things up, and then actual > > moving can be > > done. > > Thank you for working on this. Welcome! Sorry for slow reply, I am on summer vacations now, no chance for fast replies now :-) > Can you outline the plan for moving these options to the table AM to > make sure this patch is a step in the right direction? Yes, I can. First you can see the whole patch, the way it was several years ago: https://commitfest.postgresql.org/15/992/, reloptions8a.diff The things I would say is accurate for postgres ~11, it may have been changed since I last payed attention to them. So, there are three general places where options can be stored: 1. Global boolRelOpts, intRelOpts, realRelOpts, stringRelOpts in src/backend/ access/common/reloptions.c for in-core access methods. 2. custom_options array of accessable via add_bool_reloption, add_int_reloption, add_real_reloption, add_string_reloption for access methods from contribs. (See reloptions.c too) 3. And also each access method has an array of relopt_parse_elt[] that is also about reloptions. 1 and 2 are technically arrays of relopt_get, and store information what kind of options do we have. 3 is array of relopt_parse_elt[] that store information how options should be stored into binary representation. My idea was to merge relopt_get and relopt_parse_elt into one structure (in my patch it is "option_definition_basic"), each access method, that needs options, should have a set of option_definition_basic for their options (called option_definition_basic in my patch, may be should be renamed) and this set of option_definitions is the only data that is needed to parse option into binary representation. So in access method instead of am_option function we will have amrelopt_catalog function that returns "options defenition set" for this am, and this definition set is used by option parser to parse options. So, it my explanation is not quite clear, please ask questions, I will try to answer them. > I was trying to work through this problem as well[1], and there are a > few complications. > > * Which options apply to any relation (of any table AM), and which > apply to only heaps? As far as I can tell, the only one that seems > heap-specific is "fillfactor". From my point of view, each relation kind has it's own set of options. The fact, that almost every kind has a fillfactor is just a coincidence. If we try to do some optimization here, we will be buried under the complexity of it soon. So they are _different_ options just having the same name. > * Toast tables can be any AM, as well, so if we accept new reloptions > for a custom AM, we also need to accept options for toast tables of > that AM. When I wrote this patch, AM was introduced only to index relations. I do not how it is implemented for heap now, but there should be some logic in it. If toast tables has it's own AM, then option definition set should be stored there, and we should find a way to work with it, somehow. > * Implementation-wise, the bytea representation of the options is not > very easy to extend. Should we have a new text field in the catalog to > hold the custom options? I am not really understanding this question. Normally all options can be well represented as binary structure stored at bytea. I see no problem here. If we need some unusual behaviour, we can use string option with custom validation function. This should cover almost all needs I can imagine. === So it you are interested in having better option implementation, and has no ideas of your own, I would suggest to revive my patch and try to commit it. What I need first of all is a reviewer. Testing and coauthoring will also be apriciated. My original big patch, I gave you link to, have been split into several parts. The last minor part, that should be commit in advance, and have not been commit yet is https://commitfest.postgresql.org/33/2370/ If you join as a reviewer this would be splendid! :-) -- Nikolay Shaplov dh...@nataraj.su (e-mail, jabber) @dhyan:nataraj.su (matrix)
Re: pgbench logging broken by time logic changes
On 6/20/21 5:02 AM, Fabien COELHO wrote: > >> Upon reflection, that amounts to the same thing really, so yeah, >> scratch that plan. I'll defer until after that (and then I'll be >> leaning more towards the revert option). > > Sigh. I do not understand anything about the decision processus. Yes, sometimes it passeth all understanding. There will certainly be a BETA3, and in every recent year except last year there has been a BETA4. If this were core server code threatening data integrity I would be inclined to be more strict, but after all pg_bench is a utility program, and I think we can allow a little more latitude. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: PXGS vs TAP tests
Andrew Dunstan writes: > The recipe for running TAP tests in src/Makefile.global doesn't work for > the PGXS case. If you try it you get something like this: > ... > Notice those bogus settings for top_builddir, PG_REGRESS and > REGRESS_SHLIB. The attached patch fixes this bug. OK, but does the 'prove_check' macro need similar adjustments? > I removed the REGRESS_SHLIB setting altogether for the PGXS case - it's > not clear to me why we need it in a TAP test recipe at all. After some digging in the git history, it looks like it's there because of Noah's c09850992, which makes me wonder whether 017_shm.pl requires it. If so, it'd make more sense perhaps for that one test script to set up the environment variable than to have it cluttering every TAP run. (In any case, please don't push this till after beta2 is tagged. We don't need possible test instability right now.) regards, tom lane
Re: PXGS vs TAP tests
On 6/20/21 10:45 AM, Tom Lane wrote: > Andrew Dunstan writes: >> The recipe for running TAP tests in src/Makefile.global doesn't work for >> the PGXS case. If you try it you get something like this: >> ... >> Notice those bogus settings for top_builddir, PG_REGRESS and >> REGRESS_SHLIB. The attached patch fixes this bug. > OK, but does the 'prove_check' macro need similar adjustments? No, PGXS doesn't support 'make check'. In the case of TAP tests it really doesn't matter - you're not going to be running against a started server anyway. > >> I removed the REGRESS_SHLIB setting altogether for the PGXS case - it's >> not clear to me why we need it in a TAP test recipe at all. > After some digging in the git history, it looks like it's there because > of Noah's c09850992, which makes me wonder whether 017_shm.pl requires > it. If so, it'd make more sense perhaps for that one test script > to set up the environment variable than to have it cluttering every TAP > run. Yeah, I'll do some testing. > > (In any case, please don't push this till after beta2 is tagged. > We don't need possible test instability right now.) > > Yes, of course. cheers andre -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Race condition in InvalidateObsoleteReplicationSlots()
Hah, desmoxytes failed once: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=desmoxytes&dt=2021-06-19%2003%3A06%3A04 I'll revert it and investigate later. There have been no other failures. -- Álvaro Herrera39°49'30"S 73°17'W "Hay que recordar que la existencia en el cosmos, y particularmente la elaboración de civilizaciones dentro de él no son, por desgracia, nada idílicas" (Ijon Tichy)
Re: Teaching users how they can get the most out of HOT in Postgres 14
> On Jun 14, 2021, at 7:46 PM, Peter Geoghegan wrote: > > Does anyone else have an opinion on this? Of course I can easily add a > GUC. But I won't do so in the absence of any real argument in favor of > it. I'd want to see some evidence that the GUC is necessary. (For that matter, why is a per relation setting necessary?) Is there a reproducible pathological case, perhaps with a pgbench script, to demonstrate the need? I'm not asking whether there might be some regression, but rather whether somebody wants to construct a worst-case pathological case and publish quantitative results about how bad it is. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Teaching users how they can get the most out of HOT in Postgres 14
On Sun, Jun 20, 2021 at 9:22 AM Mark Dilger wrote: > I'd want to see some evidence that the GUC is necessary. (For that matter, > why is a per relation setting necessary?) Is there a reproducible > pathological case, perhaps with a pgbench script, to demonstrate the need? > I'm not asking whether there might be some regression, but rather whether > somebody wants to construct a worst-case pathological case and publish > quantitative results about how bad it is. One clear argument in favor of the VACUUM option (not so much the reloption) is that it enables certain testing scenarios. For example, I was recently using pg_visibility to do a low-level analysis of how visibility map bits were getting set with a test case that built on the BenchmarkSQL fair-use TPC-C implementation. The optimization was something that I noticed in certain scenarios -- I could have used the option of disabling it at the VACUUM command level just to get a perfectly clean slate. A small fraction of the pages in the table to not be set all-visible, which would be inconsequential to users but was annoying in the context of this particular test scenario. The way the optimization works will only ever leave an affected table in a state where the LP_DEAD items left behind would be highly unlikely to be counted by ANALYZE. They would not be counted accurately anyway, either because they're extremely few in number or because there are relatively many that are concentrated in just a few heap blocks -- that's how block-based sampling by ANALYZE works. In short, even if there really was a performance problem implied by the bypass indexes optimization, it seems unlikely that autovacuum would run in the first place to take care of it, with or without the optimization. Even if autovacuum_vacuum_scale_factor were set very aggressively. VACUUM (really autovacuum) just doesn't tend to work at that level of precision. -- Peter Geoghegan
Re: Speed up transaction completion faster after many relations are accessed in a transaction
On Sun, Jun 20, 2021 at 6:56 AM David Rowley wrote: > On Wed, 14 Aug 2019 at 19:25, David Rowley > wrote: > > For now, I'm out of ideas. If anyone else feels like suggesting > > something of picking this up, feel free. > > This is a pretty old thread, so we might need a recap: > > # Recap > > Basically LockReleaseAll() becomes slow after a large number of locks > have all been held at once by a backend. The problem is that the > LockMethodLocalHash dynahash table must grow to store all the locks > and when later transactions only take a few locks, LockReleaseAll() is > slow due to hash_seq_search() having to skip the sparsely filled > buckets in the bloated hash table. > > The following things were tried on this thread. Each one failed: > > 1) Use a dlist in LOCALLOCK to track the next and prev LOCALLOCK. > Simply loop over the dlist in LockReleaseAll(). > 2) Try dropping and recreating the dynahash table if it becomes > bloated using some heuristics to decide what "bloated" means and if > recreating is worthwhile. > > #1 failed due to concerns with increasing the size of LOCALLOCK to > store the dlist pointers. Performance regressions were seen too. > Possibly due to size increase or additional overhead from pushing onto > the dlist. > #2 failed because it was difficult to agree on what the heuristics > would be and we had no efficient way to determine the maximum number > of locks that a given transaction held at any one time. We only know > how many were still held at LockReleaseAll(). > > There were also some suggestions to fix dynahash's hash_seq_search() > slowness, and also a suggestion to try using simplehash.h instead of > dynahash.c. Unfortunately simplehash.h would suffer the same issues as > it too would have to skip over empty buckets in a sparsely populated > hash table. > > I'd like to revive this effort as I have a new idea on how to solve the > problem. > > # Background > > Over in [1] I'm trying to improve the performance of smgropen() during > recovery. The slowness there comes from the dynahash table lookups to > find the correct SMgrRelation. Over there I proposed to use simplehash > instead of dynahash because it's quite a good bit faster and far > lessens the hash lookup overhead during recovery. One problem on that > thread is that relcache keeps a pointer into the SMgrRelation > (RelationData.rd_smgr) and because simplehash moves things around > during inserts and deletes, then we can't have anything point to > simplehash entries, they're unstable. I fixed that over on the other > thread by having the simplehash entry point to a palloced > SMgrRelationData... My problem is, I don't really like that idea as it > means we need to palloc() pfree() lots of little chunks of memory. > > To fix the above, I really think we need a version of simplehash that > has stable pointers. Providing that implementation is faster than > dynahash, then it will help solve the smgropen() slowness during > recovery. > > # A new hashtable implementation > > I ended up thinking of this thread again because the implementation of > the stable pointer hash that I ended up writing for [1] happens to be > lightning fast for hash table sequential scans, even if the table has > become bloated. The reason the seq scans are so fast is that the > implementation loops over the data arrays, which are tightly packed > and store the actual data rather than pointers to the data. The code > does not need to loop over the bucket array for this at all, so how > large that has become is irrelevant to hash table seq scan > performance. > > The patch stores elements in "segments" which is set to some power of > 2 value. When we run out of space to store new items in a segment, we > just allocate another segment. When we remove items from the table, > new items reuse the first unused item in the first segment with free > space. This helps to keep the used elements tightly packed. A > segment keeps a bitmap of used items so that means scanning all used > items is very fast. If you flip the bits in the used_item bitmap, > then you get a free list for that segment, so it's also very fast to > find a free element when inserting a new item into the table. > > I've called the hash table implementation "generichash". It uses the > same preprocessor tricks as simplehash.h does and borrows the same > linear probing code that's used in simplehash. The bucket array just > stores the hash value and a uint32 index into the segment item that > stores the data. Since segments store a power of 2 items, we can > easily address both the segment number and the item within the segment > from the single uint32 index value. The 'index' field just stores a > special value when the bucket is empty. No need to add another field > for that. This means the bucket type is just 8 bytes wide. > > One thing I will mention about the new hash table implementation is > that GH_ITEMS_PER_SEGMENT is, by default, set to 256. This means > that's th
Re: Race condition in InvalidateObsoleteReplicationSlots()
I wrote: > Hmm ... desmoxytes has failed this test once, out of four runs since > it went in: > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=desmoxytes&dt=2021-06-19%2003%3A06%3A04 I studied this failure a bit more, and I think the test itself has a race condition. It's doing # freeze walsender and walreceiver. Slot will still be active, but walreceiver # won't get anything anymore. kill 'STOP', $senderpid, $receiverpid; $logstart = get_log_size($node_primary3); advance_wal($node_primary3, 4); ok(find_in_log($node_primary3, "to release replication slot", $logstart), "walreceiver termination logged"); The string it's looking for does show up in node_primary3's log, but not for another second or so; we can see instances of the following poll_query_until query before that happens. So the problem is that there is no interlock to ensure that the walreceiver terminates before this find_in_log check looks for it. You should be able to fix this by adding a retry loop around the find_in_log check (which would likely mean that you don't need to do multiple advance_wal iterations here). However, I agree with reverting the test for now and then trying again after beta2. regards, tom lane
Re: PXGS vs TAP tests
On 6/20/21 10:56 AM, Andrew Dunstan wrote: > On 6/20/21 10:45 AM, Tom Lane wrote: > >>> I removed the REGRESS_SHLIB setting altogether for the PGXS case - it's >>> not clear to me why we need it in a TAP test recipe at all. >> After some digging in the git history, it looks like it's there because >> of Noah's c09850992, which makes me wonder whether 017_shm.pl requires >> it. If so, it'd make more sense perhaps for that one test script >> to set up the environment variable than to have it cluttering every TAP >> run. > > Yeah, I'll do some testing. > > > Tests pass with the attached patch, which puts the setting in the Makefile for the recovery tests. The script itself doesn't need any changing. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com diff --git a/src/Makefile.global.in b/src/Makefile.global.in index 8f05840821..ea0ed854a0 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -444,16 +444,24 @@ with_temp_install = \ ifeq ($(enable_tap_tests),yes) +ifndef PGXS define prove_installcheck rm -rf '$(CURDIR)'/tmp_check $(MKDIR_P) '$(CURDIR)'/tmp_check -cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' REGRESS_SHLIB='$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl) +cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl) endef +else # PGXS case +define prove_installcheck +rm -rf '$(CURDIR)'/tmp_check +$(MKDIR_P) '$(CURDIR)'/tmp_check +cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(top_builddir)' PG_REGRESS='$(top_builddir)/src/test/regress/pg_regress' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl) +endef +endif # PGXS define prove_check rm -rf '$(CURDIR)'/tmp_check $(MKDIR_P) '$(CURDIR)'/tmp_check -cd $(srcdir) && TESTDIR='$(CURDIR)' $(with_temp_install) PGPORT='6$(DEF_PGPORT)' PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' REGRESS_SHLIB='$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl) +cd $(srcdir) && TESTDIR='$(CURDIR)' $(with_temp_install) PGPORT='6$(DEF_PGPORT)' PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl) endef else diff --git a/src/test/recovery/Makefile b/src/test/recovery/Makefile index 96442ceb4e..ebf957ae94 100644 --- a/src/test/recovery/Makefile +++ b/src/test/recovery/Makefile @@ -15,6 +15,9 @@ subdir = src/test/recovery top_builddir = ../../.. include $(top_builddir)/src/Makefile.global +REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX) +export REGRESS_SHLIB + check: $(prove_check)
Re: a path towards replacing GEQO with something better
Hi, I stumbled across this which may be of interest to this topic and GEQO alternative. The main creator/author of Neo and Bao (ML for Query Optimizer) Ryan Marcus (finishing Postdoc and looking for job) recently posted [1] about Bao for distributed systems. But what was interesting was the links he had to a 2020 Neo YouTube [2] which discussed better cardinality estimation / 90% less errors (vs. Postgres 10) only improved query latency by 2-3%, and other MLs made worse in other scenarios. Other interesting takeaways from the video (summary): PostgreSQL Query Optimizer – 40k LOC. PG10 70% worse/slower than Oracle. PG has 3 major flaws in QO, 1 fixed in PG11. Neo 10-25% better than PG QO after 30hr training (using GPU). Neo drops to 10% better if 3 flaws were / could be fixed. MS SQL – 1 million LOC. Oracle – 45-55 FTEs working on it. No LOC given by Oracle. Appear to focus on TPC-DS. NEO better than Oracle after 60hr training (using GPU). Humans and hand tuning will always beat ML. I.e. Neo (and Bao) good for those who cannot afford a fulltime DBA doing query optimizing. Bao – follow-on work from Neo. “This is a prototype implementation of Bao for PostgreSQL. Bao is a learned query optimizer that learns to "steer" the PostgreSQL optimizer by issuing coarse-grained query hints. For more information about Bao” BAO GitHub here [3] and is AGPLv3 license (not sure if that’s good or bad). Bao drawbacks… (but may not matter from a GEQO perspective??) “Of course, Bao does come with some drawbacks. Bao causes query optimization to take a little bit more time (~300ms), requiring quite a bit more computation. We studied this overhead in our SIGMOD paper. For data warehouse workloads, which largely consists of long-running, resource intensive queries, Bao’s increased overhead is hardly noticeable. However, for workloads with a lot of short running queries, like OLTP workloads, this might not be the case. We are currently working on new approaches to mitigate that problem – so stay tuned!” [1] https://rmarcus.info/blog/2021/06/17/bao-distributed.html [2] https://www.youtube.com/watch?v=lMb1yNbIopc Cardinality errors impact on latency - Starting at 8:00, interesting at 10:10 approx. [3] https://github.com/learnedsystems/baoforpostgresql -- Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html
Re: Different compression methods for FPI
> 17 июня 2021 г., в 11:44, Michael Paquier написал(а): > > I have worked more on that today and finished with two patches I have some small questions. 1. + report_invalid_record(record, "image at %X/%X compressed with %s not supported, block %d", + (uint32) (record->ReadRecPtr >> 32), + (uint32) record->ReadRecPtr, + "lz4", + block_id); Can we point out to user that the problem is in the build? Also, maybe %s can be inlined to lz4 in this case. 2. > const char *method = "???"; Maybe we can use something like "unknown" for unknown compression methods? Or is it too long string for waldump output? 3. Can we exclude lz4 from config if it's not supported by the build? Thanks! Best regards, Andrey Borodin.
Re: Race condition in InvalidateObsoleteReplicationSlots()
I wrote: > I studied this failure a bit more, and I think the test itself has > a race condition. It's doing > > # freeze walsender and walreceiver. Slot will still be active, but walreceiver > # won't get anything anymore. > kill 'STOP', $senderpid, $receiverpid; > $logstart = get_log_size($node_primary3); > advance_wal($node_primary3, 4); > ok(find_in_log($node_primary3, "to release replication slot", $logstart), > "walreceiver termination logged"); Actually ... isn't there a second race, in the opposite direction? IIUC, the point of this is that once we force some WAL to be sent to the frozen sender/receiver, they'll be killed for failure to respond. But the advance_wal call is not the only possible cause of that; a background autovacuum for example could emit some WAL. So I fear it's possible for the 'to release replication slot' message to come out before we capture $logstart. I think you need to capture that value before the kill not after. I also suggest that it wouldn't be a bad idea to make the find_in_log check more specific, by including the expected PID and perhaps the expected slot name in the string. The full message in primary3's log looks like 2021-06-19 05:24:36.221 CEST [60cd636f.362648:12] LOG: terminating process 3548959 to release replication slot "rep3" and I don't understand why we wouldn't match on the whole message text. (I think doing so will also reveal that what we are looking for here is the walsender pid, not the walreceiver pid, and thus that the description in the ok() call is backwards. Or maybe we do want to check the walreceiver side, in which case we are searching the wrong postmaster's log?) regards, tom lane
Reducing the cycle time for CLOBBER_CACHE_ALWAYS buildfarm members
I had an idea for another way to attack $SUBJECT, now that we have the ability to adjust debug_invalidate_system_caches_always at runtime. Namely, that a lot of time goes into repeated initdb runs (especially if the TAP tests are enabled); but surely we learn little from CCA initdb runs after the first one. We could trim this fat by: (1) Instead of applying CLOBBER_CACHE_ALWAYS as a compile option, add "debug_invalidate_system_caches_always = 1" to the buildfarm's "extra_config" options, which are added to postgresql.conf after initdb. Thus, initdb will run without that option but all the actual test cases will have it. (2) To close the testing gap that now we have *no* CCA coverage of initdb runs, adjust either the buildfarm's initdb-only steps or initdb's 001_initdb.pl TAP script to set "debug_invalidate_system_caches_always = 1" in one of the runs. I think we can make that happen so far as the bootstrap backend is concerned by including "-c debug_invalidate_system_caches_always=1" on its command line; but of course initdb itself has no way to be told to do that. I think we could invent a "-c NAME=VALUE" switch for initdb to tell it to pass down that switch to its child backends. Then there'd have to be some way to tell the calling tests whether to do that. (3) Since this only works in v14 and up, older branches would have to fall back to -DCLOBBER_CACHE_ALWAYS. Perhaps we could improve the buildfarm client script so that buildfarm owners just configure "clobber_cache_testing => 1" and then the script would do the right branch-dependent thing. Of course, we could eliminate the need for branch-dependent logic if we cared to back-patch the addition of the debug_invalidate_system_caches_always GUC, but that's probably a bridge too far. It looks to me like this would cut around an hour off of the roughly-a-day cycle times of the existing CCA animals. None of them run any TAP tests, presumably because that would make their cycle time astronomical, but maybe this change will help make that practical. Thoughts? regards, tom lane
Re: unnesting multirange data types
On Sun, Jun 20, 2021 at 11:09 AM Noah Misch wrote: > On Sat, Jun 19, 2021 at 10:05:09PM -0400, Tom Lane wrote: > > Alexander Korotkov writes: > > > I also don't feel comfortable hurrying with unnest part to beta2. > > > According to the open items wiki page, there should be beta3. Does > > > unnest part have a chance for beta3? > > > > Hm. I'd prefer to avoid another forced initdb after beta2. On the > > other hand, it's entirely likely that there will be some other thing > > that forces that; in which case there'd be no reason not to push in > > the unnest feature as well. > > > > I'd say let's sit on the unnest code for a little bit and see what > > happens. > > I think $SUBJECT can't simultaneously offer too little to justify its own > catversion bump and also offer enough to bypass feature freeze. If multirange > is good without $SUBJECT, then $SUBJECT should wait for v15. Otherwise, the > matter of the catversion bump should not delay commit of $SUBJECT. FWIW, there is a patch implementing just unnest() function. -- Regards, Alexander Korotkov multirange_unnest-v7.patch Description: Binary data
Re: Reducing the cycle time for CLOBBER_CACHE_ALWAYS buildfarm members
On 6/20/21 6:10 PM, Tom Lane wrote: > (3) Since this only works in v14 and up, older branches would > have to fall back to -DCLOBBER_CACHE_ALWAYS. Perhaps we could > improve the buildfarm client script so that buildfarm owners > just configure "clobber_cache_testing => 1" and then the script > would do the right branch-dependent thing. Maybe. Let's see what you come up with. > > Of course, we could eliminate the need for branch-dependent > logic if we cared to back-patch the addition of the > debug_invalidate_system_caches_always GUC, but that's probably > a bridge too far. Yeah, I think so. > > It looks to me like this would cut around an hour off of the > roughly-a-day cycle times of the existing CCA animals. None > of them run any TAP tests, presumably because that would make > their cycle time astronomical, but maybe this change will help > make that practical. > It might. I'm fairly sure there are a lot of repetitive cycles wasted in the TAP tests, quite apart from initdb. We've become rather profligate in our use of time and resources. cheers adrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: seawasp failing, maybe in glibc allocator
On Sun, Jun 20, 2021 at 10:59 PM Andres Freund wrote:
> I think this should be part of the earlier loop? Once
> LLVMOrcAbsoluteSymbols() is called that owns the reference, so there
> doesn't seem to be a reason to increase the refcount only later?
Right, that makes sense. Here's a patch like that.
Looking at their release schedule on https://llvm.org/, I see we have
a gamble to make. They currently plan to cut RC1 at the end of July,
and to release in late September (every second LLVM major release
coincides approximately with a PG major release). Option 1: wait
until we branch for 14, and then push this to master so that at least
seawasp can get back to looking for new problems, and then back-patch
only after they release (presumably in time for our November
releases). If their API change sticks, PostgreSQL crashes and gives
weird results with the initial release of LLVM 13 until our fix comes
out. Option 2: get ahead of their release and get this into 14 +
August back branch releases based on their current/RC behaviour. If
they decide to revert the change before the final release, we'll leak
symbol names because we hold an extra reference, until we can fix
that.
For the last round of changes[1], there was a similar when-to-act
question, but that was a doesn't-compile-anymore API change, whereas
this is a silent demons-might-fly-out-of-your-nose API change.
[1]
https://www.postgresql.org/message-id/flat/20201016011244.pmyvr3ee2gbzplq4%40alap3.anarazel.de
From 6bb6eeaead0a80663504bbec7bbb1a32f08cfd92 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Mon, 21 Jun 2021 09:41:43 +1200
Subject: [PATCH] Adjust for LLVM 13 API change.
LLVM 13 (not yet released, but due out in a few months) has changed the
semantics of LLVMOrcAbsoluteSymbols(), so that we now need to bump a
reference count on the symbol names we give it to avoid a double-free.
Discussion: https://postgr.es/m/CA%2BhUKGLEy8mgtN7BNp0ooFAjUedDTJj5dME7NxLU-m91b85siA%40mail.gmail.com
---
src/backend/jit/llvm/llvmjit.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c
index 71029a39a9..df691cbf1c 100644
--- a/src/backend/jit/llvm/llvmjit.c
+++ b/src/backend/jit/llvm/llvmjit.c
@@ -1106,6 +1106,9 @@ llvm_resolve_symbols(LLVMOrcDefinitionGeneratorRef GeneratorObj, void *Ctx,
{
const char *name = LLVMOrcSymbolStringPoolEntryStr(LookupSet[i].Name);
+#if LLVM_VERSION_MAJOR > 12
+ LLVMOrcRetainSymbolStringPoolEntry(LookupSet[i].Name);
+#endif
symbols[i].Name = LookupSet[i].Name;
symbols[i].Sym.Address = llvm_resolve_symbol(name, NULL);
symbols[i].Sym.Flags.GenericFlags = LLVMJITSymbolGenericFlagsExported;
--
2.30.2
Re: seawasp failing, maybe in glibc allocator
Thomas Munro writes: > Looking at their release schedule on https://llvm.org/, I see we have > a gamble to make. They currently plan to cut RC1 at the end of July, > and to release in late September (every second LLVM major release > coincides approximately with a PG major release). Option 1: wait > until we branch for 14, and then push this to master so that at least > seawasp can get back to looking for new problems, and then back-patch > only after they release (presumably in time for our November > releases). If their API change sticks, PostgreSQL crashes and gives > weird results with the initial release of LLVM 13 until our fix comes > out. Option 2: get ahead of their release and get this into 14 + > August back branch releases based on their current/RC behaviour. If > they decide to revert the change before the final release, we'll leak > symbol names because we hold an extra reference, until we can fix > that. If that's an accurate characterization of the tradeoff, I have little difficulty in voting for #2. A crash is strictly worse than a memory leak. Besides which, I've heard little indication that they might revert. regards, tom lane
Re: seawasp failing, maybe in glibc allocator
On Sun, Jun 20, 2021 at 11:01 PM Andres Freund wrote: > On 2021-06-19 10:12:03 -0400, Tom Lane wrote: > > Is a compile-time conditional really going to be reliable? See nearby > > arguments about compile-time vs run-time checks for libpq features. > > It's not clear to me how tightly LLVM binds its headers and running > > code. > > It should be fine (and if not we have plenty other places it'd be > problematic). LLVM embeds the version between user of llvm and the > library version in some symbol, so if there's a sufficient mismatch > it'll cause link time issues. Of course that only works for major > versions, but that shouldn't be an issue here. I looked into this a bit. On the usual Unixoid server OSes the first line of defence is that the major version is baked into the library name to support parallel installation of major versions, so our llvmjit.so is linked against eg libLLVM-13.so.1 (all controlled by llvm-config), and then there are further defences like versioned symbols, LLVM_13 etc on some platforms. Curiously, they skip this scheme for Macs (see their AddLLVM.cmake file) and Windows. So of course I wanted to try to see if I could make it break in the way Tom imagined, on a Mac. There, I use MacPorts, and it has separate packages for major versions, for example "llvm-12", much like other distros. The package maintainers put libLLVM.dylib (LLVM project's choice for this platform) into different paths under .../libexec/llvm-$VERSION/ (package maintainer's choice), and there is a tool to select the current default (alarm bells ringing at this point). The first observation is that the Mach-O "compatibility version" is 1.0.0 on all the .dylibs, so yeah, that mechanism isn't going to save you, but ... it turns out to be a moot question for now because, to my surprise, we're statically linking LLVM into our llvmjit.so on that platform. That turns out to be because llvm-config --libs won't spit out dynamic link options if it can't find a library name with the version embedded in it. I see now that Brew's maintainers take it on themselves to create that symlink[1] (unlike MacPorts'), so ... erm, could be trouble there, I dunno because I don't want to install that, but if so, maybe they asked for it? I guess that none of this stuff really matters for real world non-hacker users, who are probably using an installer that ships its own copy of the thing. I expect it'll be the same on Windows when we eventually support LLVM there. /me closes Macintosh [1] https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/llvm.rb#L175
Re: Optionally automatically disable logical replication subscriptions on error
On Sat, Jun 19, 2021 at 11:44 PM Mark Dilger wrote: > > > > > On Jun 19, 2021, at 3:17 AM, Amit Kapila wrote: > > > > Right, but there are things that could be common from the design > > perspective. > > I went to reconcile my patch with that from [1] only to discover there is no > patch on that thread. Is there one in progress that I can see? I will submit the patch. > > I don't mind trying to reconcile this patch with what you're discussing in > [1], but I am a bit skeptical about [1] becoming a reality and I don't want > to entirely hitch this patch to that effort. This can be committed with or > without any solution to the idea in [1]. The original motivation for this > patch was that the TAP tests don't have a great way to deal with a > subscription getting into a fail-retry infinite loop, which makes it harder > for me to make progress on [2]. That doesn't absolve me of the > responsibility of making this patch a good one, but it does motivate me to > keep it simple. There was a discussion that the skipping transaction patch would also need to have a feature that tells users the details of the last failure transaction such as its XID, timestamp, action etc. In that sense, those two patches might need the common infrastructure that the apply workers leave the error details somewhere so that the users can see it. > > For example, why is it mandatory to update this conflict > > ( error) information in the system catalog instead of displaying it > > via some stats view? > > The catalog must be updated to disable the subscription, so placing the error > information in the same row doesn't require any independent touching of the > catalogs. Likewise, the catalog must be updated to re-enable the > subscription, so clearing the error from that same row doesn't require any > independent touching of the catalogs. > > The error information does not *need* to be included in the catalog, but > placing the information in any location that won't survive server restart > leaves the user no information about why the subscription got disabled after > a restart (or crash + restart) happens. > > Furthermore, since v2 removed the "disabled_by_error" field in favor of just > using subenabled + suberrmsg to determine if the subscription was > automatically disabled, not having the information in the catalog would make > it ambiguous whether the subscription was manually or automatically disabled. Is it really useful to write only error message to the system catalog? Even if we see the error message like "duplicate key value violates unique constraint “test_tab_pkey”” on the system catalog, we will end up needing to check the server log for details to properly resolve the conflict. If the user wants to know whether the subscription is disabled manually or automatically, the error message on the system catalog might not necessarily be necessary. > For the problem in [1], having the xid is more important than it is in my > patch, because the user is expected in [1] to use the xid as a handle. But > that seems like an odd interface to me. Imagine that a transaction on the > publisher side inserted a batch of data, and only a subset of that data > conflicts on the subscriber side. What advantage is there in skipping the > entire transaction? Wouldn't the user rather skip just the problematic rows? > I understand that on the subscriber side it is difficult to do so, but if > you are going to implement this sort of thing, it makes more sense to allow > the user to filter out data that is problematic rather than filtering out > xids that are problematic, and the filter shouldn't just be an in-or-out > filter, but rather a mapping function that can redirect the data someplace > else or rewrite it before inserting or change the pre-existing conflicting > data prior to applying the problematic data or whatever. That's a huge > effort, of course, but if the idea in [1] goes in that direction, I don't > want my patch to have already added an xid field which ultimately nobody > wants. > The feature discussed in that thread is meant to be a repair tool for the subscription in emergency cases when something that should not have happened happened. I guess that resolving row (or column) level conflict should be done in another way, for example, by defining policies for each type of conflict. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: Optionally automatically disable logical replication subscriptions on error
> On Jun 20, 2021, at 7:17 PM, Masahiko Sawada wrote: > > I will submit the patch. Great, thanks! > There was a discussion that the skipping transaction patch would also > need to have a feature that tells users the details of the last > failure transaction such as its XID, timestamp, action etc. In that > sense, those two patches might need the common infrastructure that the > apply workers leave the error details somewhere so that the users can > see it. Right. Subscription on error triggers would need that, too, if we wrote them. > Is it really useful to write only error message to the system catalog? > Even if we see the error message like "duplicate key value violates > unique constraint “test_tab_pkey”” on the system catalog, we will end > up needing to check the server log for details to properly resolve the > conflict. If the user wants to know whether the subscription is > disabled manually or automatically, the error message on the system > catalog might not necessarily be necessary. We can put more information in there. I don't feel strongly about it. I'll wait for your patch to see what infrastructure you need. > The feature discussed in that thread is meant to be a repair tool for > the subscription in emergency cases when something that should not > have happened happened. I guess that resolving row (or column) level > conflict should be done in another way, for example, by defining > policies for each type of conflict. I understand that is the idea, but I'm having trouble believing it will work that way in practice. If somebody has a subscription that has gone awry, what reason do we have to believe there will only be one transaction that will need to be manually purged? It seems just as likely that there would be a million transactions that need to be purged, and creating an interface for users to manually review them and keep or discard on a case by case basis seems unworkable. Sure, you might have specific cases where the number of transactions to purge is small, but I don't like designing the feature around that assumption. All the same, I'm looking forward to seeing your patch! — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Windows' copy of ICU
Hi, I noticed a striking similarity between the collation versions reported by Windows and ICU, and found my way to this new system copy of ICU (C APIs only) that you can use on recent enough Windows[1]. Not planning to do anything with that observation myself but it seemed interesting enough to share... [1] https://docs.microsoft.com/en-us/windows/win32/intl/international-components-for-unicode--icu-
Re: Optionally automatically disable logical replication subscriptions on error
On Sat, Jun 19, 2021 at 8:14 PM Mark Dilger wrote: > > > On Jun 19, 2021, at 3:17 AM, Amit Kapila wrote: > > Also, why not also log the xid of the failed > > transaction? > > We could also do that. Reading [1], it seems you are overly focused on > user-facing xids. The errdetail in the examples I've been using for testing, > and the one mentioned in [1], contain information about the conflicting data. > I think users are more likely to understand that a particular primary key > value cannot be replicated because it is not unique than to understand that a > particular xid cannot be replicated. (Likewise for permissions errors.) For > example: > > 2021-06-18 16:25:20.139 PDT [56926] ERROR: duplicate key value violates > unique constraint "s1_tbl_unique" > 2021-06-18 16:25:20.139 PDT [56926] DETAIL: Key (i)=(1) already exists. > 2021-06-18 16:25:20.139 PDT [56926] CONTEXT: COPY tbl, line 2 > > This tells the user what they need to clean up before they can continue. > Telling them which xid tried to apply the change, but not the change itself > or the conflict itself, seems rather unhelpful. So at best, the xid is an > additional piece of information. I'd rather have both the ERROR and DETAIL > fields above and not the xid than have the xid and lack one of those two > fields. Even so, I have not yet included the DETAIL field because I didn't > want to bloat the catalog. > I never said that we don't need the error information. I think we need xid along with other things. > For the problem in [1], having the xid is more important than it is in my > patch, because the user is expected in [1] to use the xid as a handle. But > that seems like an odd interface to me. Imagine that a transaction on the > publisher side inserted a batch of data, and only a subset of that data > conflicts on the subscriber side. What advantage is there in skipping the > entire transaction? Wouldn't the user rather skip just the problematic rows? > I think skipping some changes but not others can make the final transaction data inconsistent. Say, we have a case where, in a transaction after insert, there is an update or delete on the same row, then we might silently skip such updates/deletes unless the same row is already present in the subscriber. I think skipping the entire transaction based on user instruction would be safer than skipping some changes that lead to an error. -- With Regards, Amit Kapila.
Re: Optionally automatically disable logical replication subscriptions on error
On Mon, Jun 21, 2021 at 7:56 AM Mark Dilger wrote: > > > On Jun 20, 2021, at 7:17 PM, Masahiko Sawada wrote: > > > > I will submit the patch. > > Great, thanks! > > > There was a discussion that the skipping transaction patch would also > > need to have a feature that tells users the details of the last > > failure transaction such as its XID, timestamp, action etc. In that > > sense, those two patches might need the common infrastructure that the > > apply workers leave the error details somewhere so that the users can > > see it. > > Right. Subscription on error triggers would need that, too, if we wrote them. > > > Is it really useful to write only error message to the system catalog? > > Even if we see the error message like "duplicate key value violates > > unique constraint “test_tab_pkey”” on the system catalog, we will end > > up needing to check the server log for details to properly resolve the > > conflict. If the user wants to know whether the subscription is > > disabled manually or automatically, the error message on the system > > catalog might not necessarily be necessary. > > I think the two key points are (a) to define exactly what all information is required to be logged on error, (b) where do we want to store the information based on requirements. I see that for (b) Mark is inclined to use the existing catalog table. I feel that is worth considering but not sure if that is the best way to deal with it. For example, if we store that information in the catalog, we might need to consider storing it both in pg_subscription and pg_subscription_rel, otherwise, we might overwrite the errors as I think what is happening in the currently proposed patch. The other possibilities could be to define a new catalog table to capture the error information or log the required information via stats collector and then the user can see that info via some stats view. > > We can put more information in there. I don't feel strongly about it. I'll > wait for your patch to see what infrastructure you need. > > > The feature discussed in that thread is meant to be a repair tool for > > the subscription in emergency cases when something that should not > > have happened happened. I guess that resolving row (or column) level > > conflict should be done in another way, for example, by defining > > policies for each type of conflict. > > I understand that is the idea, but I'm having trouble believing it will work > that way in practice. If somebody has a subscription that has gone awry, > what reason do we have to believe there will only be one transaction that > will need to be manually purged? > Because currently, we don't proceed after an error unless it is resolved. Why do you think there could be multiple such transactions? -- With Regards, Amit Kapila.
RE: locking [user] catalog tables vs 2pc vs logical rep
On Sunday, June 20, 2021 9:50 PM I wrote: > On Sunday, June 20, 2021 3:23 PM Amit Kapila > wrote: > > On Sun, Jun 20, 2021 at 9:28 AM osumi.takami...@fujitsu.com > > wrote: > > > * doc/src/sgml/logicaldecoding.sgml > ... > > > > > > Now we have the four paren supplementary descriptions, not to make > > > users miss any other [user] catalog tables. > > > Because of this, the built output html gives me some redundant > > > impression, for that parts. Accordingly, couldn't we move them to > > > outside of the itemizedlist section in a simple manner ? > > > > > > For example, to insert a sentence below the list, after removing the > > > paren descriptions in the listitem, which says "Note that those > > > commands that can cause deadlock apply to not only explicitly > > > indicated system catalog tables above but also any other [user] catalog > table." > > > > Sounds reasonable to me. /but also any other/but also to any other/, > > to seems to be missing in the above line. Kindly send an update patch. > Excuse me, I don't understand the second sentence. > I wrote "but also" clause in my example. > > Also, attached the patch for the change to the HEAD. I've updated the patch to follow the correction Amit-san mentioned. Please check. Best Regards, Takamichi Osumi v4-0001-Doc-Update-caveats-in-synchronous-logical-replica.patch Description: v4-0001-Doc-Update-caveats-in-synchronous-logical-replica.patch
Re: PG 14 release notes, first draft
On Tue, Jun 15, 2021 at 12:01:00PM +0900, Michael Paquier wrote: > On Tue, Jun 15, 2021 at 11:49:21AM +0900, Masahiko Sawada wrote: > > On Tue, Jun 15, 2021 at 10:36 AM Bruce Momjian wrote: > >> OK, but I need more information on how users will see a difference based > >> on this commit: > > +1. That would be good to have in the release notes. > > > I think that since with this commit the server on Windows can handle a > > file over 4GB, COPY FROM loading data from an over 4GB file and > > pg_dump dumping a large table work now. > > Segment files or WAL files larger than 4GB also gain from that. > Anything for which we may finish to do a stat() on benefits from this > change if running on Windows. For pg_dump, a workaround in PG <= 13 > was to use --no-sync as the stat() failure came from files with a size > larger than 4GB. That's rather sad as that means sacrifying > durability for more usability :( OK, I went with this text and put it in the Source Code section since it applies to several layers of Postgres. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion. diff --git a/doc/src/sgml/release-14.sgml b/doc/src/sgml/release-14.sgml index 53221ab8c1..28d0e1396b 100644 --- a/doc/src/sgml/release-14.sgml +++ b/doc/src/sgml/release-14.sgml @@ -3553,6 +3553,25 @@ Author: Peter Eisentraut + + +Allow Windows to properly handle files larger than four gigabytes +(Juan José Santamaría Flecha) + + + +For example this allows COPY, WAL +files, and relation segment files to be larger than four gigabytes. + + + + +
Re: Optionally automatically disable logical replication subscriptions on error
On Mon, Jun 21, 2021 at 12:09 PM Amit Kapila wrote: > > On Mon, Jun 21, 2021 at 7:56 AM Mark Dilger > wrote: > > > > > On Jun 20, 2021, at 7:17 PM, Masahiko Sawada > > > wrote: > > > > > > I will submit the patch. > > > > Great, thanks! > > > > > There was a discussion that the skipping transaction patch would also > > > need to have a feature that tells users the details of the last > > > failure transaction such as its XID, timestamp, action etc. In that > > > sense, those two patches might need the common infrastructure that the > > > apply workers leave the error details somewhere so that the users can > > > see it. > > > > Right. Subscription on error triggers would need that, too, if we wrote > > them. > > > > > Is it really useful to write only error message to the system catalog? > > > Even if we see the error message like "duplicate key value violates > > > unique constraint “test_tab_pkey”” on the system catalog, we will end > > > up needing to check the server log for details to properly resolve the > > > conflict. If the user wants to know whether the subscription is > > > disabled manually or automatically, the error message on the system > > > catalog might not necessarily be necessary. > > > > > I think the two key points are (a) to define exactly what all > information is required to be logged on error, When it comes to the patch for skipping transactions, it would somewhat depend on how users specify transactions to skip. On the other hand, for this patch, the minimal information would be whether the subscription is disabled automatically by the server. > (b) where do we want to > store the information based on requirements. I see that for (b) Mark > is inclined to use the existing catalog table. I feel that is worth > considering but not sure if that is the best way to deal with it. For > example, if we store that information in the catalog, we might need to > consider storing it both in pg_subscription and pg_subscription_rel, > otherwise, we might overwrite the errors as I think what is happening > in the currently proposed patch. The other possibilities could be to > define a new catalog table to capture the error information or log the > required information via stats collector and then the user can see > that info via some stats view. This point is also related to the point whether or not that information needs to last after the server crash (and restart). When it comes to the patch for skipping transactions, there was a discussion that we don’t necessarily need it since the tools will be used in rare cases. But for this proposed patch, I guess it would be useful if it does. It might be worth considering doing a different way for each patch. For example, we send the details of last failure transaction to the stats collector while updating subenabled to something like “automatically-disabled” instead of to just “false” (or using another column to show the subscriber is disabled automatically by the server). Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: Maintaining a list of pgindent commits for "git blame" to ignore
On Thu, Mar 18, 2021 at 4:32 PM Peter Geoghegan wrote: > On Thu, Mar 18, 2021 at 4:05 PM Tom Lane wrote: > > b5d69b7c22ee4c44b8bb99cfa0466ffaf3b5fab9 # Sun Jun 7 16:57:08 2020 -0400 > > # pgindent run prior to branching v13. > > > > which is easy to make from "git log" or "git show" output. (Because > > of the precedent of those tools, I'd rather write the commit hash > > before the rest of the entry.) > > WFM. What do you think of the attached? I prefer the ISO date style myself, so I went with that. Note that I have included "Modify BufferGetPage() to prepare for "snapshot too old" feature", as well as "Revert no-op changes to BufferGetPage()". I've noticed that those two particular commits cause unhelpful noise when I run "git blame" on access method code. I see problems with these commits often enough to matter. The latter commit cleanly reverted the former after only 12 days, so ignoring both seems okay to me. Everything else should be either pgindent/perltidy related or reformat-dat-files related. -- Peter Geoghegan pgindent-git-blame-ignore-revs Description: Binary data
Re: [PATCH v3 1/1] Fix detection of preadv/pwritev support for OSX.
Hi, Do we see any solution to this issue? or using the older SDK is the way to go? On Thu, May 20, 2021 at 2:04 PM Dave Page wrote: > > > On Tue, Mar 30, 2021 at 6:58 AM Tom Lane wrote: > >> Thomas Munro writes: >> > I'll move it when committing. I'll let this patch sit for another day >> > to see if any other objections show up. >> >> FWIW, I remain fairly strongly against this, precisely because of the >> point that it requires us to start using a randomly different >> feature-probing technology anytime Apple decides that they're going to >> implement some standard API that they didn't before. Even if it works >> everywhere for preadv/pwritev (which we won't know in advance of >> buildfarm testing, and maybe not then, since detection failures will >> probably be silent), it seems likely that we'll hit some case in the >> future where this interacts badly with some other platform's weirdness. >> We haven't claimed in the past to support MACOSX_DEPLOYMENT_TARGET, >> and I'm not sure we should start now. How many people actually care >> about that? >> > > I missed this earlier - it's come to my attention through a thread on the > -packagers list. Adding my response on that thread here for this audience: > > The ability to target older releases with a newer SDK is essential for > packages such as the EDB PostgreSQL installers and the pgAdmin community > installers. It's very difficult (sometimes impossible) to get older OS > versions on new machines now - Apple make it very hard to download old > versions of macOS (some can be found, others not), and they won't always > work on newer hardware anyway so it's really not feasible to have all the > build machines running the oldest version that needs to be supported. > > FYI, the pgAdmin and PG installer buildfarms have > -mmacosx-version-min=10.12 in CFLAGS etc. to handle this, which is > synonymous with MACOSX_DEPLOYMENT_TARGET. We've been successfully building > packages that way for a decade or more. > > -- > Dave Page > Blog: https://pgsnake.blogspot.com > Twitter: @pgsnake > > EDB: https://www.enterprisedb.com > > -- Sandeep Thakkar
Re: Optionally automatically disable logical replication subscriptions on error
> On Jun 20, 2021, at 8:09 PM, Amit Kapila wrote: > > Because currently, we don't proceed after an error unless it is > resolved. Why do you think there could be multiple such transactions? Just as one example, if the subscriber has a unique index that the publisher lacks, any number of transactions could add non-unique data that then fails to apply on the subscriber. My patch took the view that the user should figure out how to get the subscriber side consistent with the publisher side, but if you instead take the approach that problematic commits should be skipped, it would seem that arbitrarily many such transactions could be committed on the publisher side. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [bug?] Missed parallel safety checks, and wrong parallel safety
On Thu, Jun 17, 2021 at 8:29 PM Robert Haas wrote: > > On Thu, Jun 17, 2021 at 4:54 AM Amit Kapila wrote: > > I have responded about heavy-weight locking stuff in my next email [1] > > and why I think the approach I mentioned will work. I don't deny that > > I might be missing something here. > > > > [1] - > > https://www.postgresql.org/message-id/CAA4eK1%2BT2CWqp40YqYttDA1Skk7wK6yDrkCD5GZ80QGr5ze-6g%40mail.gmail.com > > I mean I saw that but I don't see how it addresses the visibility > issue. > I thought if we scan a system catalog using DirtySnapshot, then we should be able to find such a relation. But, if the system catalog is updated after our scan then surely we won't be able to see it and in that case, we won't be able to send invalidation. Now, say if the rel is not visible to us because of the snapshot we used or due to some race condition then we won't be able to send the invalidation but why we want to consider it worse than the case where we miss such invalidations (invalidations due to change of parallel-safe property) when the insertion into relation is in-progress. > There could be a relation that is not visible to your snapshot > and upon which AccessExclusiveLock is held which needs to be > invalidated. You can't lock it because it's AccessExclusiveLock'd > already. > Yeah, the session in which we are doing Alter Function won't be able to lock it but it will wait for the AccessExclusiveLock on the rel to be released because it will also try to acquire it before sending invalidation. -- With Regards, Amit Kapila.
Re: PG 14 release notes, first draft
On Mon, Jun 21, 2021 at 12:50 PM Bruce Momjian wrote: > > On Tue, Jun 15, 2021 at 12:01:00PM +0900, Michael Paquier wrote: > > On Tue, Jun 15, 2021 at 11:49:21AM +0900, Masahiko Sawada wrote: > > > On Tue, Jun 15, 2021 at 10:36 AM Bruce Momjian wrote: > > >> OK, but I need more information on how users will see a difference based > > >> on this commit: > > > > +1. That would be good to have in the release notes. > > > > > I think that since with this commit the server on Windows can handle a > > > file over 4GB, COPY FROM loading data from an over 4GB file and > > > pg_dump dumping a large table work now. > > > > Segment files or WAL files larger than 4GB also gain from that. > > Anything for which we may finish to do a stat() on benefits from this > > change if running on Windows. For pg_dump, a workaround in PG <= 13 > > was to use --no-sync as the stat() failure came from files with a size > > larger than 4GB. That's rather sad as that means sacrifying > > durability for more usability :( > > OK, I went with this text and put it in the Source Code section since it > applies to several layers of Postgres. Thanks! I got the parse error after applying the patch: release-14.sgml:3562: parser error : Input is not proper UTF-8, indicate encoding ! Bytes: 0xE9 0x20 0x53 0x61 (Juan Jos Santamara Flecha) ^ Is that a problem with my environment? Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: PG 14 release notes, first draft
On Mon, Jun 21, 2021 at 01:57:32PM +0900, Masahiko Sawada wrote: > > OK, I went with this text and put it in the Source Code section since it > > applies to several layers of Postgres. > > Thanks! > > I got the parse error after applying the patch: > > release-14.sgml:3562: parser error : Input is not proper UTF-8, > indicate encoding ! > Bytes: 0xE9 0x20 0x53 0x61 > (Juan Jos Santamara Flecha) > ^ > > Is that a problem with my environment? I don't know, but it builds here and properly shows here: https://momjian.us/pgsql_docs/release-14.html -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: PG 14 release notes, first draft
On Fri, Jun 18, 2021 at 08:47:21PM -0700, Peter Geoghegan wrote: > On Mon, Jun 14, 2021 at 1:11 PM Bruce Momjian wrote: > > FYI, the most recent PG 14 relnote doc build is at: > > > > https://momjian.us/pgsql_docs/release-14.html > > I just pushed a commit that makes the existing vacuum_index_cleanup > reloption and INDEX_CLEANUP VACUUM parameter support disabling the > "Allow vacuum to skip index vacuuming when the number of removable > index entries is insignificant" behavior. This should be mentioned in > the release notes. Agreed. I updated the PG 14 release notes to be current as of today, and adjusted your item --- patch attached. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion. diff --git a/doc/src/sgml/release-14.sgml b/doc/src/sgml/release-14.sgml index 28d0e1396b..33b7bf7d57 100644 --- a/doc/src/sgml/release-14.sgml +++ b/doc/src/sgml/release-14.sgml @@ -6,7 +6,7 @@ Release date: - 2021-??-?? (AS OF 2021-05-15) + 2021-??-?? (AS OF 2021-06-20) @@ -424,12 +424,14 @@ Author: Tom Lane Require custom server - variable names to match the pattern used for unquoted - SQL identifiers (Tom Lane) + variable names to use only character which are valid for + unquoted SQL identifiers (Tom Lane) @@ -691,12 +693,20 @@ Author: Peter Eisentraut Allow vacuum to skip index vacuuming when the number of removable index entries is insignificant (Masahiko Sawada, Peter Geoghegan) + + +The vacuum parameter INDEX_CLEANUP has a +new default of auto to enable this optimization. +
Re: PG 14 release notes, first draft
On Mon, Jun 21, 2021 at 2:07 PM Bruce Momjian wrote: > > On Mon, Jun 21, 2021 at 01:57:32PM +0900, Masahiko Sawada wrote: > > > OK, I went with this text and put it in the Source Code section since it > > > applies to several layers of Postgres. > > > > Thanks! > > > > I got the parse error after applying the patch: > > > > release-14.sgml:3562: parser error : Input is not proper UTF-8, > > indicate encoding ! > > Bytes: 0xE9 0x20 0x53 0x61 > > (Juan Jos Santamara Flecha) > > ^ > > > > Is that a problem with my environment? > > I don't know, but it builds here and properly shows here: > > https://momjian.us/pgsql_docs/release-14.html Maybe it's my environmental problem. Thanks anyway! Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: Optionally automatically disable logical replication subscriptions on error
On Mon, Jun 21, 2021 at 10:24 AM Mark Dilger wrote: > > > On Jun 20, 2021, at 8:09 PM, Amit Kapila wrote: > > > > Because currently, we don't proceed after an error unless it is > > resolved. Why do you think there could be multiple such transactions? > > Just as one example, if the subscriber has a unique index that the publisher > lacks, any number of transactions could add non-unique data that then fails > to apply on the subscriber. > Then also it will fail on the first such conflict, so even without your patch, the apply worker corresponding to the subscription won't be able to proceed after the first error, it won't lead to multiple failing xids. However, I see a different case where there could be multiple failing xids and that can happen during initial table sync where multiple workers failed due to some error. I am not sure your patch would be able to capture all such failed transactions because you are recording this information in pg_subscription and not in pg_subscription_rel. -- With Regards, Amit Kapila.
Re: Optionally automatically disable logical replication subscriptions on error
> On Jun 20, 2021, at 8:09 PM, Amit Kapila wrote:
>
> (a) to define exactly what all
> information is required to be logged on error, (b) where do we want to
> store the information based on requirements.
I'm not sure it has to be stored anywhere durable. I have a patch in the works
to do something like:
create function foreign_key_insert_violation_before() returns conflict_trigger
as $$
BEGIN
RAISE NOTICE 'elevel: %', TG_ELEVEL:
RAISE NOTICE 'sqlerrcode: %', TG_SQLERRCODE:
RAISE NOTICE 'message: %', TG_MESSAGE:
RAISE NOTICE 'detail: %', TG_DETAIL:
RAISE NOTICE 'detail_log: %', TG_DETAIL_LOG:
RAISE NOTICE 'hint: %', TG_HINT:
RAISE NOTICE 'schema: %', TG_SCHEMA_NAME:
RAISE NOTICE 'table: %', TG_TABLE_NAME:
RAISE NOTICE 'column: %', TG_COLUMN_NAME:
RAISE NOTICE 'datatype: %', TG_DATATYPE_NAME:
RAISE NOTICE 'constraint: %', TG_CONSTRAINT_NAME:
-- do something useful to prepare for retry of transaction
-- which raised a foreign key violation
END
$$ language plpgsql;
create function foreign_key_insert_violation_after() returns conflict_trigger
as $$
BEGIN
-- do something useful to cleanup after retry of transaction
-- which raised a foreign key violation
END
$$ language plpgsql;
create conflict trigger regress_conflict_trigger_insert on regress_conflictsub
before foreign_key_violation
when tag in ('INSERT')
execute procedure foreign_key_insert_violation_before();
create conflict trigger regress_conflict_trigger_insert on regress_conflictsub
after foreign_key_violation
when tag in ('INSERT')
execute procedure foreign_key_insert_violation_after();
The idea is that, for subscriptions that have conflict triggers defined, the
apply will be wrapped in a PG_TRY()/PG_CATCH() block. If it fails, the
ErrorData will be copied in the ConflictTriggerContext, and then the
transaction will be attempted again, but this time with any BEFORE and AFTER
triggers applied. The triggers could then return a special result indicating
whether the transaction should be permanently skipped, applied, or whatever.
None of the data needs to be stored anywhere non-transient, as it just gets
handed to the triggers.
I think the other patch is a subset of this functionality, as using this system
to create triggers which query a table containing transactions to be skipped
would be enough to get the functionality you've been discussing. But this
system could also do other things, like modify data. Admittedly, this is akin
to a statement level trigger and not a row level trigger, so a number of things
you might want to do would be hard to do from this. But perhaps the equivalent
of row level triggers could also be written?
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: Optionally automatically disable logical replication subscriptions on error
> On Jun 20, 2021, at 10:11 PM, Amit Kapila wrote: > > Then also it will fail on the first such conflict, so even without > your patch, the apply worker corresponding to the subscription won't > be able to proceed after the first error, it won't lead to multiple > failing xids. I'm not sure we're talking about the same thing. I'm saying that if the user is expected to clear each error manually, there could be many such errors for them to clear. It may be true that the second error doesn't occur on the subscriber side until after the first is cleared, but that still leaves the user having to clear one after the next until arbitrarily many of them coming from the publisher side are cleared. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Added schema level support for publication.
Thanks for the comments.
On Fri, Jun 18, 2021 at 5:25 PM Ajin Cherian wrote:
>
> On Thu, Jun 17, 2021 at 12:41 AM vignesh C wrote:
>
> > Thanks for reporting it, the attached patch is a rebased version of
> > the patch with few review comment fixes, I will reply with the comment
> > fixes to the respective mails.
> >
>
> I've applied the patch, it applies cleand and ran "make check" and
> tests run fine.
>
> Some comments for patch 1:
>
> In the commit message, some grammar mistakes:
>
> "Changes was done in
> pg_dump to handle pubtype updation in pg_publication table while the database
> gets upgraded."
>
> -- change to --
>
> Changes were done in
> pg_dump to handle pubtype updation in pg_publication table while the database
> gets upgraded.
>
I will modify this.
> =
>
> Prototypes present in pg_publication.h was moved to publicationcmds.h so
> that minimal datastructures ...
>
> - change to --
>
> Prototypes present in pg_publication.h were moved to publicationcmds.h so
> that minimal datastructures ..
>
>
I will modify this.
>
> In patch 1:
>
> In getObjectDescription()
>
> + if (!nspname)
> + {
> + pfree(pubname);
> + pfree(nspname);
> + ReleaseSysCache(tup);
>
> Why free nspname if it is NULL?
>
> Same comment in getObjectIdentityParts()
I will modify this.
>
>
> In GetAllTablesPublicationRelations()
>
> + ScanKeyData key[2];
> TableScanDesc scan;
> HeapTuple tuple;
> List*result = NIL;
> + int keycount = 0;
>
> classRel = table_open(RelationRelationId, AccessShareLock);
>
> - ScanKeyInit(&key[0],
> + ScanKeyInit(&key[keycount++],
>
> Here you have init key[1], but the code below in the same function
> inits key[0]. I am not sure if this will affect that code below.
>
> if (pubviaroot)
> {
> ScanKeyInit(&key[0],
> Anum_pg_class_relkind,
> BTEqualStrategyNumber, F_CHAREQ,
> CharGetDatum(RELKIND_PARTITIONED_TABLE));
I felt this is ok as we specify the keycount to be 1, so only the
key[0] will be used. Thoughts?
ScanKeyInit(&key[0],
Anum_pg_class_relkind,
BTEqualStrategyNumber, F_CHAREQ,
CharGetDatum(RELKIND_PARTITIONED_TABLE));
scan = table_beginscan_catalog(classRel, 1, key);
> =
>
> in UpdatePublicationTypeTupleValue():
>
> + tup = heap_modify_tuple(tup, RelationGetDescr(rel), values, nulls,
> + replaces);
> +
> + /* Update the catalog. */
> + CatalogTupleUpdate(rel, &tup->t_self, tup);
>
> Not sure if this tup needs to be freed or if the memory context will
> take care of it.
I felt this is ok, as the cleanup is handled in the caller function
"AlterPublication", thoughts?
/* Cleanup. */
heap_freetuple(tup);
table_close(rel, RowExclusiveLock);
I will post an update patch for the fixes shortly.
Regards,
Vignesh
Re: [PATCH v3 1/1] Fix detection of preadv/pwritev support for OSX.
On Mon, Jun 21, 2021 at 4:32 PM Sandeep Thakkar wrote: > Do we see any solution to this issue? or using the older SDK is the way to go? > On Thu, May 20, 2021 at 2:04 PM Dave Page wrote: >> The ability to target older releases with a newer SDK is essential for >> packages such as the EDB PostgreSQL installers and the pgAdmin community >> installers. It's very difficult (sometimes impossible) to get older OS >> versions on new machines now - Apple make it very hard to download old >> versions of macOS (some can be found, others not), and they won't always >> work on newer hardware anyway so it's really not feasible to have all the >> build machines running the oldest version that needs to be supported. >> >> FYI, the pgAdmin and PG installer buildfarms have -mmacosx-version-min=10.12 >> in CFLAGS etc. to handle this, which is synonymous with >> MACOSX_DEPLOYMENT_TARGET. We've been successfully building packages that way >> for a decade or more. I'm not personally against the proposed change. I'll admit there is something annoying about Apple's environment working in a way that doesn't suit traditional configure macros that have been the basis of portable software for a few decades, but when all's said and done, configure is a Unix wars era way to make things work across all the Unixes, and most of them are long gone, configure itself is on the way out, and Apple's still here, so... On a more practical note, rereading Tom's objection and Dave's counter-objection, I'm left wondering if people would be happy with a manual control for this, something you can pass to configure to stop it from using pwritev/preadv even if detected. That would at least localise the effects, avoiding the sorts of potential unintended consequences Tom mentioned.
Re: Optionally automatically disable logical replication subscriptions on error
> On Jun 20, 2021, at 10:11 PM, Amit Kapila wrote: > > However, I see a different case where there could be > multiple failing xids and that can happen during initial table sync > where multiple workers failed due to some error. I am not sure your > patch would be able to capture all such failed transactions because > you are recording this information in pg_subscription and not in > pg_subscription_rel. Right, I wasn't trying to capture everything, just enough to give the user a reasonable indication of what went wrong. My patch was designed around the idea that the user would need to figure out how to fix the subscriber side prior to re-enabling the subscription. As such, I wasn't bothered with trying to store everything, just enough to give the user a clue where to look. I don't mind if you want to store more information, and maybe that needs to be stored somewhere else. Do you believe pg_subscription_rel is a suitable location? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Optionally automatically disable logical replication subscriptions on error
On Mon, Jun 21, 2021 at 9:21 AM Masahiko Sawada wrote: > > On Mon, Jun 21, 2021 at 12:09 PM Amit Kapila wrote: > > > > On Mon, Jun 21, 2021 at 7:56 AM Mark Dilger > > wrote: > > > > > > > On Jun 20, 2021, at 7:17 PM, Masahiko Sawada > > > > wrote: > > > > > > > > I will submit the patch. > > > > > > Great, thanks! > > > > > > > There was a discussion that the skipping transaction patch would also > > > > need to have a feature that tells users the details of the last > > > > failure transaction such as its XID, timestamp, action etc. In that > > > > sense, those two patches might need the common infrastructure that the > > > > apply workers leave the error details somewhere so that the users can > > > > see it. > > > > > > Right. Subscription on error triggers would need that, too, if we wrote > > > them. > > > > > > > Is it really useful to write only error message to the system catalog? > > > > Even if we see the error message like "duplicate key value violates > > > > unique constraint “test_tab_pkey”” on the system catalog, we will end > > > > up needing to check the server log for details to properly resolve the > > > > conflict. If the user wants to know whether the subscription is > > > > disabled manually or automatically, the error message on the system > > > > catalog might not necessarily be necessary. > > > > > > > > I think the two key points are (a) to define exactly what all > > information is required to be logged on error, > > When it comes to the patch for skipping transactions, it would > somewhat depend on how users specify transactions to skip. On the > other hand, for this patch, the minimal information would be whether > the subscription is disabled automatically by the server. > True, but still there will be some information related to ERROR which we wanted the user to see unless we ask them to refer to logs for that. > > (b) where do we want to > > store the information based on requirements. I see that for (b) Mark > > is inclined to use the existing catalog table. I feel that is worth > > considering but not sure if that is the best way to deal with it. For > > example, if we store that information in the catalog, we might need to > > consider storing it both in pg_subscription and pg_subscription_rel, > > otherwise, we might overwrite the errors as I think what is happening > > in the currently proposed patch. The other possibilities could be to > > define a new catalog table to capture the error information or log the > > required information via stats collector and then the user can see > > that info via some stats view. > > This point is also related to the point whether or not that > information needs to last after the server crash (and restart). When > it comes to the patch for skipping transactions, there was a > discussion that we don’t necessarily need it since the tools will be > used in rare cases. But for this proposed patch, I guess it would be > useful if it does. It might be worth considering doing a different way > for each patch. For example, we send the details of last failure > transaction to the stats collector while updating subenabled to > something like “automatically-disabled” instead of to just “false” (or > using another column to show the subscriber is disabled automatically > by the server). > I agree that it is worth considering to have subenabled to have a tri-state (enable, disabled, automatically-disabled) value instead of just a boolean. But in this case, if the stats collector missed updating the information, the user may have to manually update the subscription and let the error happen again to see it. -- With Regards, Amit Kapila.
Re: PG 14 release notes, first draft
> On Mon, Jun 21, 2021 at 12:50 PM Bruce Momjian wrote: >> >> On Tue, Jun 15, 2021 at 12:01:00PM +0900, Michael Paquier wrote: >> > On Tue, Jun 15, 2021 at 11:49:21AM +0900, Masahiko Sawada wrote: >> > > On Tue, Jun 15, 2021 at 10:36 AM Bruce Momjian wrote: >> > >> OK, but I need more information on how users will see a difference based >> > >> on this commit: >> > >> > +1. That would be good to have in the release notes. >> > >> > > I think that since with this commit the server on Windows can handle a >> > > file over 4GB, COPY FROM loading data from an over 4GB file and >> > > pg_dump dumping a large table work now. >> > >> > Segment files or WAL files larger than 4GB also gain from that. >> > Anything for which we may finish to do a stat() on benefits from this >> > change if running on Windows. For pg_dump, a workaround in PG <= 13 >> > was to use --no-sync as the stat() failure came from files with a size >> > larger than 4GB. That's rather sad as that means sacrifying >> > durability for more usability :( >> >> OK, I went with this text and put it in the Source Code section since it >> applies to several layers of Postgres. > > Thanks! > > I got the parse error after applying the patch: > > release-14.sgml:3562: parser error : Input is not proper UTF-8, > indicate encoding ! > Bytes: 0xE9 0x20 0x53 0x61 > (Juan Jos Santamara Flecha) > ^ > > Is that a problem with my environment? Me too. I think the problem is, Bruce's patch is encoded in ISO-8859-1, not UTF-8. As far as I know PostgreSQL never encodes *.sgml files in ISO-8859-1. Anyway, attached is the Bruce's patch encoded in UTF-8. This works for me. My guess is, when Bruce attached the file, his MUA automatically changed the file encoding from UTF-8 to ISO-8859-1 (it could happen in many MUA). Also that's the reason why he does not see the problem while compiling the sgml files. In his environment release-14.sgml is encoded in UTF-8, I guess. To prevent the problem next time, it's better to change the mime type of the attached file to Application/Octet-Stream. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp master.diff Description: Binary data
Re: Optionally automatically disable logical replication subscriptions on error
On Mon, Jun 21, 2021 at 10:55 AM Mark Dilger wrote: > > > On Jun 20, 2021, at 10:11 PM, Amit Kapila wrote: > > > > However, I see a different case where there could be > > multiple failing xids and that can happen during initial table sync > > where multiple workers failed due to some error. I am not sure your > > patch would be able to capture all such failed transactions because > > you are recording this information in pg_subscription and not in > > pg_subscription_rel. > > Right, I wasn't trying to capture everything, just enough to give the user a > reasonable indication of what went wrong. My patch was designed around the > idea that the user would need to figure out how to fix the subscriber side > prior to re-enabling the subscription. As such, I wasn't bothered with > trying to store everything, just enough to give the user a clue where to look. > Okay, but the clue will be pretty random because you might end up just logging one out of several errors. > I don't mind if you want to store more information, and maybe that needs to > be stored somewhere else. Do you believe pg_subscription_rel is a suitable > location? > It won't be sufficient to store information in either pg_subscription_rel or pg_susbscription. I think if we want to store the required information in a catalog then we need to define a new catalog (pg_subscription_conflicts or something like that) with information corresponding to each rel in subscription (srsubid oid (Reference to subscription), srrelid oid (Reference to relation), ). OTOH, we can choose to send the error information to stats collector which will then be available via stat view and update system catalog to disable the subscription but there will be a risk that we might send info of failed transaction to stats collector but then fail to update system catalog to disable the subscription. -- With Regards, Amit Kapila.
Re: when the startup process doesn't (logging startup delays)
> What is DUMMY about ? If you just want to separate the "start" from "end",
> you could write:
>
> /* codes for start of operations */
> FSYNC_IN_PROGRESS
> SYNCFS_IN_PROGRESS
> ...
> /* codes for end of operations */
> FSYNC_END
> SYNCFS_END
> ...
That was by mistake and I have corrected it in the attached patch.
Thanks & Regards,
Nitin Jadhav
On Thu, Jun 17, 2021 at 6:22 PM Justin Pryzby wrote:
>
> + * Codes of the operations performed during startup process
> + */
> +typedef enum StartupProcessOp
> +{
> + SYNCFS_IN_PROGRESS,
> + FSYNC_IN_PROGRESS,
> + RECOVERY_IN_PROGRESS,
> + RESET_UNLOGGED_REL_IN_PROGRESS,
> + DUMMY,
> + SYNCFS_END,
> + FSYNC_END,
> + RECOVERY_END,
> + RESET_UNLOGGED_REL_END
> +} StartupProcessOp;
>
> What is DUMMY about ? If you just want to separate the "start" from "end",
> you could write:
>
> /* codes for start of operations */
> FSYNC_IN_PROGRESS
> SYNCFS_IN_PROGRESS
> ...
> /* codes for end of operations */
> FSYNC_END
> SYNCFS_END
> ...
>
> Or group them together like:
>
> FSYNC_IN_PROGRESS,
> FSYNC_END,
> SYNCFS_IN_PROGRESS,
> SYNCFS_END,
> RECOVERY_IN_PROGRESS,
> RECOVERY_END,
> RESET_UNLOGGED_REL_IN_PROGRESS,
> RESET_UNLOGGED_REL_END,
>
> --
> Justin
v4-0001-startup-process-progress.patch
Description: Binary data
Doc patch for Logical Replication Message Formats (PG14)
Hello Hackers,
while amending Npgsql to account for the Logical Streaming Replication
Protocol changes in PostgreSQL 14 I stumbled upon two documentation
inaccuracies in the Logical Replication Message Formats documentation
(https://www.postgresql.org/docs/devel/protocol-logicalrep-message-formats.html)
that have been introduced (or rather omitted) with the recent changes to
allow pgoutput to send logical decoding messages
(https://github.com/postgres/postgres/commit/ac4645c0157fc5fcef0af8ff571512aa284a2cec)
and to allow logical replication to transfer data in binary format
(https://github.com/postgres/postgres/commit/9de77b5453130242654ff0b30a551c9c862ed661).
1. The content of the logical decoding message in the 'Message' message
is prefixed with a length field (Int32) which isn't documented yet.
See
https://github.com/postgres/postgres/blob/69a58bfe4ab05567a8fab8bdce7f3095ed06b99c/src/backend/replication/logical/proto.c#L388
2. The TupleData may now contain the byte 'b' as indicator for binary
data which isn't documented yet. See
https://github.com/postgres/postgres/blob/69a58bfe4ab05567a8fab8bdce7f3095ed06b99c/src/include/replication/logicalproto.h#L83
and
https://github.com/postgres/postgres/blob/69a58bfe4ab05567a8fab8bdce7f3095ed06b99c/src/backend/replication/logical/proto.c#L558.
The attached documentation patch fixes both.
Best regards,
Brar
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index bc2a2feb0b..53b3f1f651 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -6498,6 +6498,18 @@ Message
+
+
+
+Int32
+
+
+
+Length of the content.
+
+
+
+
Byten
@@ -7430,6 +7442,19 @@ TupleData
+
+Or
+
+
+
+Byte1('b')
+
+
+
+Identifies the data as binary value.
+
+
+
Int32
