Re: logical changeset generation v6
logical changeset generation v6
От:
Andres Freund <andres@2ndquadrant.com>
Дата:
Hi!
Attached you can find the newest version of the logical changeset
generation patchset. Reduced by a couple of patches because the have
been committed last round. Hurray! and thanks.
The explanation of how to use the patch from last time:
http://archives.postgresql.org/message-id/20130614224817.GA19641%40awork2.anarazel.de
still holds true, so I am not going to repeat it here.
The individual patches are:
0001 wal_decoding: Allow walsender's to connect to a specific database
One logical decoding operation can only decode content from one
database at a time. Because of that the walsender needs to connect
to a specific database. The earlier "replication=on/off" parameter
now also has a valid parameter "database" which allows that.
0002 wal_decoding: Log xl_running_xact's at a higher frequency than checkpoints are done
Imo relatively unproblematic and even useful without changeset extraction.
0003 wal_decoding: Add information about a tables primary key to struct RelationData
Not much comments on this in the past. Kevin thinks we might want to
choose the best candidate key in a more elaborate manner.
0004 wal_decoding: Introduce wal decoding via catalog timetravel
The actual feature. Got cleaned up and shrunk since the last submission.
0005 wal_decoding: test_decoding: Add a simple decoding module in contrib
Example output plugin that's also used for testing.
0006 wal_decoding: pg_receivellog: Introduce pg_receivexlog equivalent for logical changes
Commandline utility to receive the changestream and manipulate slots.
0007 wal_decoding: test_logical_decoding: Add extension for easier testing of logical decoding
Allows to not only create and destroy logical slots which is part of
0005, but also receive the changestream via an SQL SRF.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: logical changeset generation v6
От:
Andres Freund <andres@2ndquadrant.com>
Дата:
On 2013-09-17 09:45:28 -0400, Peter Eisentraut wrote: > On 9/15/13 11:30 AM, Andres Freund wrote: > > On 2013-09-15 11:20:20 -0400, Peter Eisentraut wrote: > >> On Sat, 2013-09-14 at 22:49 +0200, Andres Freund wrote: > >>> Attached you can find the newest version of the logical changeset > >>> generation patchset. > >> > >> You probably have bigger things to worry about, but please check the > >> results of cpluspluscheck, because some of the header files don't > >> include header files they depend on. > > > > Hm. I tried to get that right, but it's been a while since I last > > checked. I don't regularly use cpluspluscheck because it doesn't work in > > VPATH builds... We really need to fix that. > > > > I'll push a fix for that to the git tree, don't think that's worth a > > resend in itself. > > This patch set now fails to apply because of the commit "Rename various > "freeze multixact" variables". And I am even partially guilty for that patch... Rebased patches attached. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: logical changeset generation v6
От:
Andres Freund <andres@2ndquadrant.com>
Дата:
On 2013-09-19 14:08:36 +0900, Fujii Masao wrote: > When I applied all the patches and do the compile, I got the following error: > > gcc -O0 -Wall -Wmissing-prototypes -Wpointer-arith > -Wdeclaration-after-statement -Wendif-labels > -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing > -fwrapv -g -I. -I../../../../src/include -D_GNU_SOURCE -c -o > snapbuild.o snapbuild.c > snapbuild.c:187: error: redefinition of typedef 'SnapBuild' > ../../../../src/include/replication/snapbuild.h:45: note: previous > declaration of 'SnapBuild' was here > make[4]: *** [snapbuild.o] Error 1 Hm. Somebody had reported that previously and I tried to fix it but obviously I failed. Unfortunately I don't see that warning in any of the gcc versions I have tried locally. Hopefully fixed. > When I applied only > 0001-wal_decoding-Allow-walsender-s-to-connect-to-a-speci.patch, > compiled the source, and set up the asynchronous replication, I got > the segmentation > fault. Fixed, I mismerged something, sorry for that. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: logical changeset generation v6.1
От:
Andres Freund <andres@2ndquadrant.com>
Дата:
Hi, Attached you can find an updated version of the series taking in some of the review comments (the others are queued, not ignored), including: * split of things from the big "Introduce wal decoding via ..." patch * fix the bug Steve notice where CreateLogicalDecodingContext was passed the wrong is_init = false where it should have been true * A number of smaller bugs I noticed while reviewing * Renaming of some variables, including guaranteedlyLogged ;) * Comment improvements in decode.c * rename pg_receivellog to pg_recvlogical I'll work more on the other points in the next days, so far they are clear of other big stuff. 0001 wal_decoding: Allow walsender's to connect to a specific database - as before 0002 wal_decoding: Log xl_running_xact's at a higher frequency than checkpoints are done - as before 0003 wal_decoding: Add information about a tables primary key to struct RelationData - as before 0004 wal_decoding: Add wal_level = logical and log data required for logical decoding - splitof patch that contains the wal format changes including the addition of a new wal_level option 0005 wal_decoding: Add option to treat additional tables as catalog tables - Option to treat user defined table as a catalog table which means it can be accessed during logical decoding from an output plugin 0006 wal_decoding: Introduce wal decoding via catalog timetravel - The guts of changeset extraction, without a user interface 0007 wal_decoding: logical changeset extraction walsender interface - splitof patch containing the walsender changes, which allow to receive the changeset data in a streaming fashion, supporting sync rep and such fancy things 0008 wal_decoding: Only peg the xmin horizon for catalog tables during logical decoding - splitof optimization which reduces the pain 06 introduces by pegging the xmin horizon to the smallest of the logical decoding slots. Now it's pegged differently for data tables than from catalog tables 0009 wal_decoding: test_decoding: Add a simple decoding module in contrib - Example output plugin which is also used in tests 0010 wal_decoding: pg_recvlogical: Introduce pg_receivexlog equivalent for logical changes - renamed client for the walsender interface 0011 wal_decoding: test_logical_decoding: Add extension for easier testing of logical decoding - SQL SRF to get data from a decoding slot, also used as a vehicle for tests 0012 wal_decoding: design document v2.4 and snapshot building design doc v0.5 Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: logical changeset generation v6.1
От:
Andres Freund <andres@2ndquadrant.com>
Дата:
On 2013-09-27 16:35:53 +0100, Thom Brown wrote: > On 27 September 2013 16:14, Andres Freund wrote: > > Hi, > > > > Attached you can find an updated version of the series taking in some of > > the review comments (the others are queued, not ignored), including: > > * split of things from the big "Introduce wal decoding via ..." patch > > * fix the bug Steve notice where CreateLogicalDecodingContext was passed > > the wrong is_init = false where it should have been true > > * A number of smaller bugs I noticed while reviewing > > * Renaming of some variables, including guaranteedlyLogged ;) > > * Comment improvements in decode.c > > * rename pg_receivellog to pg_recvlogical > > > > I'll work more on the other points in the next days, so far they are > > clear of other big stuff. > > > > > > 0001 wal_decoding: Allow walsender's to connect to a specific database > > - as before > > > > 0002 wal_decoding: Log xl_running_xact's at a higher frequency than checkpoints are done > > - as before > > > > 0003 wal_decoding: Add information about a tables primary key to struct RelationData > > - as before > > > > 0004 wal_decoding: Add wal_level = logical and log data required for logical decoding > > - splitof patch that contains the wal format changes including the > > addition of a new wal_level option > > > > 0005 wal_decoding: Add option to treat additional tables as catalog tables > > - Option to treat user defined table as a catalog table which means it > > can be accessed during logical decoding from an output plugin > > > > 0006 wal_decoding: Introduce wal decoding via catalog timetravel > > - The guts of changeset extraction, without a user interface > > > > 0007 wal_decoding: logical changeset extraction walsender interface > > - splitof patch containing the walsender changes, which allow to receive > > the changeset data in a streaming fashion, supporting sync rep and > > such fancy things > > > > 0008 wal_decoding: Only peg the xmin horizon for catalog tables during logical decoding > > - splitof optimization which reduces the pain 06 introduces by pegging > > the xmin horizon to the smallest of the logical decoding slots. Now > > it's pegged differently for data tables than from catalog tables > > > > 0009 wal_decoding: test_decoding: Add a simple decoding module in contrib > > - Example output plugin which is also used in tests > > > > 0010 wal_decoding: pg_recvlogical: Introduce pg_receivexlog equivalent for logical changes > > - renamed client for the walsender interface > > > > 0011 wal_decoding: test_logical_decoding: Add extension for easier testing of logical decoding > > - SQL SRF to get data from a decoding slot, also used as a vehicle for > > tests > > > > 0012 wal_decoding: design document v2.4 and snapshot building design doc v0.5 > > I'm encountering a make error: Gah. Lastminute changes. Always the same... Updated patch attached. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: logical changeset generation v6.2
От:
Andres Freund <andres@2ndquadrant.com>
Дата:
Hi, The series from friday was a bit too buggy - obviously I was too tired. So here's a new one: * fix pg_recvlogical makefile (Thanks Steve) * fix two commits not compiling properly without later changes (Thanks Kevin) * keep track of commit timestamps * fix bugs with option passing in test_logical_decoding * actually parse option values in test_decoding instead of just using the option name * don't use anonymous structs in unions. That's compiler specific (msvc and gcc) before C11 on which we can't rely. That unfortunately will break output plugins because ReorderBufferChange need to qualify old/new tuples now * improve error handling/cleanup in test_logical_decoding * some minor cleanups Patches attached, git tree updated. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: logical changeset generation v6.1
От:
Andres Freund <andres@2ndquadrant.com>
Дата:
Hi, On 2013-10-01 10:07:19 -0400, Robert Haas wrote: > - It seems that HeapSatisfiesHOTandKeyUpdate is now > HeapSatisfiesHOTandKeyandCandidateKeyUpdate. Considering I think this > was merely HeapSatisfiesHOTUpdate a year ago, it's hard not to be > afraid that something unscalable is happening to this function. On a > related node, any overhead added here costs broadly; I'm not sure if > there's enough to worry about. I haven't changed anything here - ISTM so far nobody had a better suggestion. > - RelationIsDoingTimetravel is still a crappy name. How about > RelationRequiredForLogicalDecoding? And maybe the reloption > treat_as_catalog_table can become required_for_logical_decoding. Hm. I don't really like the name, required seems to imply that it's necessary to turn this on to get data replicated in that relation. How about "accessible_during_logical_decoding" or "user_catalog_table"? The latter would allow us to use it to add checks for user relations used in indexes which need a treatment similar to enums. > All in all, it seems to me that we shouldn't try to punt. Maybe we > should have something that works like ALTER TABLE name CLUSTER ON > index_name to configure which index should be used for logical > replication. Possibly this same syntax could be used as ALTER > MATERIALIZED VIEW to set the candidate key for that case. How about using the current logic by default but allow to tune it additionally with an option like that? So, attached is the new version: Changes: * Fix issues you noticed except the above * Handle multixacts on system tables * Logical slots now are checksummed and contain a version and length * Improve logic for increasing the "restart lsn", the point where we start to read WAL to decode from next time round * Wait for xids in snapbuild, during the initial build * s/RelationIsDoingTimetravel/RelationRequiredForLogicalDecoding/ * test_logical_decoding: confirm reception of changes at the end * prohibit rewriting schema changes for treat_as_catalog_table relations * add tests for dropping/adding primary/candidate keys * PROCESS_INTERRUPTS whene reading wal for SQL SRF * cleanup old serialized snapshots at check/restart points * Add more isolationtester changes Todo: * rename treat_as_catalog_table, after agreeing on the new name * rename remaining timetravel function names * restrict SuspendDecodingSnapshots usage to RelationInitPhysicalAddr, that ought to be enough. * add InLogicalDecoding() function. * throw away older data when reading xl_running_xacts records, to deal with immediate shutdowns/crashes Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: logical changeset generation v6.4
От:
Andres Freund <andres@2ndquadrant.com>
Дата:
Hi, Attached you can find version 6.4 of the patchset: * reordered so the patches Robert wants to apply first are first * renamed treat_as_catalog_table to user_catalog_table * renamed RelationRequiredForLogicalDecoding to RelationIsAccessibleInLogicalDecoding * moved two hunks to better fitting patches I am working on the longer TODOs from the last version now, but they don't affect the first patches. Greetings, Andres Freund PS: git rebase -i -x /path/to/testscript is cool -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: logical changeset generation v6.2
От:
Andres Freund <andres@2ndquadrant.com>
Дата:
On 2013-10-14 15:51:14 +0200, Andres Freund wrote: > > > It'd probably not hurt to redo those benchmarks to make sure... > > > > Yes, I think it would be good to characterize it more precisely than > > "a bit", so people know what to expect. > > A "bit" was below the 3% range for loops of adding columns. > > So, any tests you'd like to see? > * loop around CREATE TABLE/DROP TABLE > * loop around ALTER TABLE ... ADD COLUMN > * loop around CREATE FUNCTION/DROP FUNCTION So, see the attatched benchmark skript. I've always done using a disk bound and a memory bound (using eatmydata, preventing fsyncs) run. * unpatched run, wal_level = hot_standby, eatmydata * unpatched run, wal_level = hot_standby * patched run, wal_level = hot_standby, eatmydata * patched run, wal_level = hot_standby * patched run, wal_level = logical, eatmydata * patched run, wal_level = logical Based on those results, there's no difference above noise for wal_level=hot_standby, with or without the patch. With wal_level=logical there's a measurable increase in wal traffic (~12-17%), but no performance decrease above noise. >From my POV that's ok, those are really crazy catalog workloads. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: logical changeset generation v6.2
От:
Andres Freund <andres@2ndquadrant.com>
Дата:
On 2013-10-21 16:15:58 +0200, Andres Freund wrote: > On 2013-10-21 09:32:12 -0400, Robert Haas wrote: > > On Fri, Oct 18, 2013 at 2:26 PM, Andres Freund wrote: > > > I know of the following solutions: > > > 1) Don't allow VACUUM FULL on catalog tables if wal_level = logical. > > > 2) Make VACUUM FULL prevent DDL and then wait till all changestreams > > > have decoded up to the current point. > > > 3) don't delete the old relfilenode for VACUUM/CLUSTERs of system tables > > > if there are life decoding slots around, instead delegate that > > > responsibility to the slot management. > > > 4) Store both (cmin, cmax) for catalog tuples. > > > > > > I bascially think only 1) and 4) are realistic. And 1) sucks. > > > > > > I've developed a prototype for 4) and except currently being incredibly > > > ugly, it seems to be the most promising approach by far. My trick to > > > store both cmin and cmax is to store cmax in t_hoff managed space when > > > wal_level = logical. > > > > In my opinion, (4) is too ugly to consider. I think that if we start > > playing games like this, we're opening up the doors to lots of subtle > > bugs and future architectural pain that will be with us for many, many > > years to come. I believe we will bitterly regret any foray into this > > area. > > Hm. After looking at the required code - which you obviously cannot have > yet - it's not actually too bad. Will post a patch implementing it later. > > I don't really buy the architectural argument since originally cmin/cmax > *were* both stored. It's not something we're just inventing now. We just > optimized that away but now have discovered that's not always a good > idea and thus don't always use the optimization. > > The actual decoding code shrinks by about 200 lines using this logic > which is a hint that it's not a bad idea. So, here's a preliminary patch to see how this would look. It'd be great of you comment if you still think it's a completel no-go. If it were for real, it'd need to be split and some minor things would need to get adjusted, but I think it's easier to review it seing both sides at once. Greetings, Andres Freund PS: The patch is ontop of a new git push, but for review that shouldn't matter. -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: logical changeset generation v6.4
От:
Andres Freund <andres@2ndquadrant.com>
Дата:
On 2013-10-21 20:16:29 +0200, Andres Freund wrote:
> On 2013-10-18 20:50:58 +0200, Andres Freund wrote:
> > How about modifying the selection to go from:
> > * all rows if ALTER TABLE ... REPLICA IDENTITY NOTHING|FULL;
> > * index chosen by ALTER TABLE ... REPLICA IDENTITY USING indexname
> > * [later, maybe] ALTER TABLE ... REPLICA IDENTITY (cola, colb)
>
> Current draft is:
> ALTER TABLE ... REPLICA IDENTITY NOTHING|FULL|DEFAULT
> ALTER TABLE ... REPLICA IDENTITY USING INDEX ...;
>
> which leaves the door open for
>
> ALTER TABLE ... REPLICA IDENTITY USING '(' column_name_list ')';
>
> Does anybody have a strong feeling about requiring support for CREATE
> TABLE for this?
Attached is a patch ontop of master implementing this syntax. It's not
wired up to the changeset extraction patch yet as I am not sure whether
others agree about the storage.
pg_class grew a 'relreplident' char, storing:
* 'd' default
* 'n' nothing
* 'f' full
* 'i' index with indisreplident set, or default if index has been
dropped
pg_index grew a 'indisreplident' bool indicating it is set as the
replica identity for a replident = 'i' relation.
Both changes shouldn't change the width of the affected relations, they
should reuse existing padding.
Does somebody prefer a different storage?
pg_dump support, psql completion, regression tests and minimal docs
included.
I am not 100% clear what the best way to handle
ALTER TABLE some_table REPLICA IDENTITY USING INDEX someindex;
DROP INDEX someindex;
is. Currently that's supposed to have the same effect as having
relreplident = 'd'.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Re: logical changeset generation v6.5
От:
Andres Freund <andres@2ndquadrant.com>
Дата:
Hi, Attached to this mail and in the xlog-decoding-rebasing-remapping branch in my git[1] repository you can find the next version of the patchset that: * Fixes full table rewrites of catalog tables using the method Robert prefers (which is to log rewrite mappings to disk) * Extract the REPLICA IDENTITY as configured with ALTER TABLE for the old tuple for UPDATEs and DELETEs * Much better support for synchronous replication * Better resource cleanup (as in we need less local WAL available) * Lots of smaller fixes The change around REPLICA IDENTITY is *incompatible* to older output plugins since we now log tuples using the table's TupleDesc, not the indexes. Robert, I'd be very grateful if you could have a look at patch 0007 implementing what we've discussed. I kept it separate to make it easier to look at it in isolation, but I think in the end it partially should be merged into the wal_level=logical patch. I still think the "wide cmin/cmax" solution is more elegant and has wider applicability, but this works as well although it's about 5 times the code. Comments? [1]: http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=summary Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: logical changeset generation v6.5
От:
Andres Freund <andres@2ndquadrant.com>
Дата:
On 2013-11-08 17:11:58 -0500, Peter Eisentraut wrote: > On 11/8/13, 3:03 PM, Robert Haas wrote: > > On Fri, Nov 8, 2013 at 12:38 PM, Robert Haas wrote: > >> On Tue, Nov 5, 2013 at 10:21 AM, Andres Freund wrote: > >>> Attached to this mail and in the xlog-decoding-rebasing-remapping branch > >>> in my git[1] repository you can find the next version of the patchset that: > >> > >> I have pushed patches #1 and #2 from this series as a single commit, > >> after some editing. > > > > And I've also pushed patch #13, which is an almost-totally-unrelated > > improvement that has nothing to do with logical replication, but is > > useful all the same. > > Please fix this new compiler warning: > > pg_regress_ecpg.c: In function ‘main’: > pg_regress_ecpg.c:170:2: warning: passing argument 3 of ‘regression_main’ from incompatible pointer type [enabled by default] > In file included from pg_regress_ecpg.c:19:0: > ../../../../src/test/regress/pg_regress.h:55:5: note: expected ‘init_function’ but argument is of type ‘void (*)(void)’ Hrmpf... I usually run something akin to # make -j3 -s && (cd contrib && make -j3 -s) and then in a separate step # make -s check-world this is so I see compiler warnings before drowning them in check-world's output. But ecpg/test isn't built during make in src/interfaces/ecpg, but just during make check there. ISTM ecpg's regression tests should be built (not run!) during $(recurse) not just during make check. Patch towards that end attached. Also attached is the fix for the compilation warning itself. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: logical changeset generation v6.6
От:
Andres Freund <andres@2ndquadrant.com>
Дата:
Hi, Changes since last version: * fixes around the logging of toasted columns for the REPLICA IDENTITY in UPDATE/DELETE. Found due to a question of Robert's. * Initial documentation for the additional wal_level, but that will require additional links once further patches of the series are committed. * Comment, elog/ereport, indentation impovements in many of the patches * Add the isolationtester tests to "make check" contrib/test_logical_decoding and introduce "installcheck-force" that forces an installcheck run even though it requires special configuration parameters. * the heap rewrite checkpoint code now skips over files not named "map-*" instead of complaining if it cannot sscanf() the filename. * pg_stat_logical_decoding system view: 'renamed' the numeric 'database' column in 'dboid' and added a join to pg_database * Remove several FIXMEs by implementing support for dropping data of transactions that were running before a crash. * Add CRC32 to snapbuild state files Questions: * Should we rename (INIT|START|FREE)_LOGICAL_REPLICATION into *_LOGICAL_DECODING? * Should we rename FREE_LOGICAL_REPLICATION into STOP_LOGICAL_REPLICATION? stop_logical_replication() currently is the SQL level function... Todo: * Implement timeline handling. We need to switch timelines when extracting changes on a standby. I think we need to readTimeLineHistory() and then liOfPointInHistory() for every segment. * Once guc and recovery.conf are merged, we might want to support using recovery_command to gather older wal files. I am starting to be rather happy with the state of the patch. 01 wal_decoding: Add wal_level = logical and log data required for logical decoding 02 wal_decoding: Log xl_running_xact's at a higher frequency than checkpoints are done 03 wal_decoding: Add option to use user defined tables as catalog tables 04 wal_decoding: Introduce wal decoding via catalog timetravel 05 wal_decoding: Implement VACUUM FULL/CLUSTER support via rewrite maps * should probably be merged with 04, kept separate for review 06 wal_decoding: Only peg the xmin horizon for catalog tables during logical decoding 07 wal_decoding: Allow walsender's to connect to a specific database 08 wal_decoding: logical changeset extraction walsender interface 09 wal_decoding: test_decoding: Add a simple decoding module in contrib 10 wal_decoding: pg_recvlogical: Introduce pg_receivexlog equivalent for logical changes 11 wal_decoding: test_logical_decoding: Add extension for easier testing of logical decoding 12 wal_decoding: design document v2.4 and snapshot building design doc v0.5 13 wal_decoding: Temporarily add logical decoding regression tests to everything * shouldn't be committed, but it's useful for testing Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: logical changeset generation v6.6
От:
Andres Freund <andres@2ndquadrant.com>
Дата:
On 2013-11-12 19:24:39 +0100, Andres Freund wrote: > On 2013-11-12 13:18:19 -0500, Robert Haas wrote: > > On Tue, Nov 12, 2013 at 12:50 PM, Andres Freund wrote: > > > Completely agreed. As evidenced by the fact that the current change > > > doesn't update all relevant comments & code. I wonder if we shouldn't > > > leave the function the current way and just add a new function for the > > > new behaviour. > > > The hard thing with that would be coming up with a new > > > name. IsSystemRelationId() having a different behaviour than > > > IsSystemRelation() seems strange to me, so just keeping that and > > > adapting the callers seems wrong to me. > > > IsInternalRelation()? IsCatalogRelation()? > > > > Well, I went through and looked at the places that were affected by > > this and I tend to think that most places will be happier with the new > > definition. > > I agree that many if not most want the new definition. > > > If there are call sites that want the existing test, maybe we should > > have IsRelationInSystemNamespace() for that, and reserve > > IsSystemRelation() for the test as to whether it's a bona fide system > > catalog. > > The big reason that I think we do not want the new behaviour for all is: > > * NB: TOAST relations are considered system relations by this test > * for compatibility with the old IsSystemRelationName function. > * This is appropriate in many places but not all. Where it's not, > * also check IsToastRelation. > > the current state of things would allow to modify toast relations in > some places :/ So, I think I found a useful defintion of IsSystemRelation() that fixes many of the issues with moving relations to pg_catalog: Continue to treat all pg_toast.* relations as system tables, but only consider initdb created relations in pg_class. I've then added IsCatalogRelation() which has a narrower definition of system relations, namely, it only counts toast tables if they are a catalog's toast table. This allows far more actions on user defined relations moved to pg_catalog. Now they aren't stuck there anymore and can be renamed, dropped et al. With one curious exception: We still cannot move a relation out of pg_catalog. I've included a hunk to allow creation of indexes on relations in pg_catalog in heap_create(), indexes on catalog relations are prevented way above, but maybe that should rather be a separate commit. What do you think? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: logical changeset generation v6.7
От:
Andres Freund <andres@2ndquadrant.com>
Дата:
On 2013-11-12 18:50:33 +0100, Andres Freund wrote: > > You've actually changed the meaning of this section (and not in a good way): > > > > be set at server start. wal_level</> must be set > > - to archive</> or hot_standby</> to allow > > - connections from standby servers. > > + to archive</>, hot_standby</> or logical</> > > + to allow connections from standby servers. > > > > I think that the previous text meant that you needed archive - or, if > > you want to allow connections, hot_standby. The new text loses that > > nuance. > > Yea, that's because it was lost on me in the first place... I think that's because the nuance isn't actually in the text - note that it is talking about max_wal_senders and talking about connections *from*, not *to* standby servers. I've reformulated the wal_level paragraph and used "or higher" in several places now. Ok, so here's a rebased version of this. I tried to fix all the issues you mentioned, and it's based on the split off IsSystemRelation() patch, I've sent yesterday (included here). Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: logical changeset generation v6.8
От:
Andres Freund <andres@2ndquadrant.com>
Дата:
On 2013-12-03 15:19:26 -0500, Robert Haas wrote: > Yeah, you're right. I think the current logic will terminate when all > flags are set to false or all attribute numbers have been checked, but > it doesn't know that if HOT's been disproven then we needn't consider > further HOT columns. I think the way to fix that is to tweak this > part: > > + if (next_hot_attnum > FirstLowInvalidHeapAttributeNumber) > check_now = next_hot_attnum; > + else if (next_key_attnum > FirstLowInvalidHeapAttributeNumber) > + check_now = next_key_attnum; > + else if (next_id_attnum > FirstLowInvalidHeapAttributeNumber) > + check_now = next_id_attnum; > else > + break; > > What I think we ought to do there is change each of those criteria to > say if (hot_result && next_hot_attnum > > FirstLowInvalidHeapAttributeNumber) and similarly for the other two. > That way we consider each set a valid source of attribute numbers only > until the result flag for that set flips false. That seems to work well, yes. Updated & rebased series attached. * Rebased since the former patch 01 has been applied * Lots of smaller changes in the wal_level=logical patch * Use Robert's version of wal_level=logical, with the above fixes * Use only macros for RelationIsAccessibleInLogicalDecoding/LogicallyLogged * Moved a mit more logic into ExtractReplicaIdentity * some comment copy-editing * Bug noted by Euler fixed, testcase added * Some copy editing in later patches, nothing significant. I've primarily sent this, because I don't know of further required changes in 0001-0003. I am trying reviewing the other patches in detail atm. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: logical changeset generation v6.8
От:
Andres Freund <andres@2ndquadrant.com>
Дата:
On 2013-12-10 19:11:03 -0500, Robert Haas wrote: > Committed #1 (again). Regarding this: > > + /* XXX: we could also do this unconditionally, the space is used anyway > + if (copy_oid) > + HeapTupleSetOid(key_tuple, HeapTupleGetOid(tp)); > > I would like to put in a big +1 for doing that unconditionally. I > didn't make that change before committing, but I think it'd be a very > good idea. Patch attached. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: logical changeset generation v6
От:
Steve Singer <steve@ssinger.info>
Дата:
On 09/25/2013 11:08 AM, Andres Freund wrote: > On 2013-09-25 11:01:44 -0400, Steve Singer wrote: >> On 09/17/2013 10:31 AM, Andres Freund wrote: >>> This patch set now fails to apply because of the commit "Rename various >>> "freeze multixact" variables". >>> And I am even partially guilty for that patch... >>> >>> Rebased patches attached. >> While testing the logical replication changes against my WIP logical slony I >> am sometimes getting error messages from the WAL sender of the form: >> unexpected duplicate for tablespace X relfilenode X > Any chance you could provide a setup to reproduce the error? > The steps to build a setup that should reproduce this error are: 1. I had apply the attached patch on top of your logical replication branch so my pg_decode_init would now if it was being called as part of a INIT_REPLICATION or START_REPLICATION. Unless I have misunderstood something you probably will want to merge this fix in 2. Get my WIP for adding logical support to slony from: git@github.com:ssinger/slony1-engine.git branch logical_repl (4af1917f8418a) (My code changes to slony are more prototype level code quality than production code quality) 3. cd slony1-engine ./configure --with-pgconfigdir=/usr/local/pg94wal/bin (or whatever) make make install 4. Grab the clustertest framework JAR from https://github.com/clustertest/clustertest-framework and build up a clustertest jar file 5. Create a file slony1-engine/clustertest/conf/java.conf that contains the path to the above JAR file as a shell variable assignment: ie CLUSTERTESTJAR=/home/ssinger/src/clustertest/clustertest_git/build/jar/clustertest-coordinator.jar 6. cp clustertest/conf/disorder.properties.sample clustertest/conf/disorder.properties edit disorder.properites to have the proper values for your environment. All 6 databases can point at the same postgres instance, this test will only actually use 2 of them(so far). 7. Run the test cd clustertest ./run_all_disorder_tests.sh This involves having the slon connect to the walsender on the database test1 and replicate the data into test2 (which is a different database on the same postmaster) If this setup seems like too much effort I can request one of the commitfest VM's from Josh and get everything setup there for you. Steve >> Any ideas? > I'll look into it. Could you provide any context to what youre doing > that's being decoded? > > Greetings, > > Andres Freund >
Re: logical changeset generation v6.7
От:
Robert Haas <robertmhaas@gmail.com>
Дата:
On Thu, Nov 14, 2013 at 8:46 AM, Andres Freund wrote: > [ new patches ] Here's an updated version of patch #2. I didn't really like the approach you took in the documentation, so I revised it. Apart from that, I spent a lot of time looking at HeapSatisfiesHOTandKeyUpdate. I'm not very happy with your changes. The idea seems to be that we'll iterate through all of the HOT columns regardless, but that might be very inefficient. Suppose there are 100 HOT columns, the last one is the only key column, and only the first one has been modified. Once we look at #1 and determine that it's not HOT, we should zoom forward and skip over the next 98, and only look at the last one; your version does not behave like that. I think there's also some confusion in your version about what ends up in the attnum values: they're normally adjusted by FirstLowInvalidHeapAttributeNumber, but when bms_first_member returns -1 then they're not. But that's not a great thing, because -1 is actually a valid attribute number. I've taken a crack at rewriting this logic, and the result looks cleaner and simpler to me, but I haven't tested it beyond the fact that it passes make check. See what you think. I haven't completely reviewed every bit of this in depth yet, but it's 1:15am, so I'm going to post what I have and throw in the towel for tonight. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: logical changeset generation v6.8
От:
David Rowley <dgrowleyml@gmail.com>
Дата:
On Wed, Dec 11, 2013 at 1:11 PM, Robert Haas <robertmhaas@gmail.com> wrote:
Committed #1 (again). Regarding this:
This introduced a new compiler warning on the visual studios build:
d:\postgres\b\src\backend\utils\cache\relcache.c(3958): warning C4715: 'RelationGetIndexAttrBitmap' : not all control paths return a value [D:\Postgres\b\postgres.vcxproj]
The attached patch fixes it.
Regards
David Rowley
+ /* XXX: we could also do this unconditionally, the space is used anyway
+ if (copy_oid)
+ HeapTupleSetOid(key_tuple, HeapTupleGetOid(tp));
I would like to put in a big +1 for doing that unconditionally. I
didn't make that change before committing, but I think it'd be a very
good idea.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: logical changeset generation v6.8
От:
David Rowley <dgrowleyml@gmail.com>
Дата:
On Sat, Dec 14, 2013 at 12:12 AM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2013-12-13 20:58:24 +1300, David Rowley wrote:
> On Wed, Dec 11, 2013 at 1:11 PM, Robert Haas <robertmhaas@gmail.com> wrote:> This introduced a new compiler warning on the visual studios build:I thought we'd managed to get elog(ERROR) properly annotated as noreturn
> d:\postgres\b\src\backend\utils\cache\relcache.c(3958): warning C4715:
> 'RelationGetIndexAttrBitmap' : not all control paths return a value
> [D:\Postgres\b\postgres.vcxproj]
>
> The attached patch fixes it.
on msvc as well?
It looks like this is down to the elog macro, where the elevel is being assigned to a variable elevel_ then we're only doing pg_unreachable(); if elevel_ >= ERROR. The compiler must not be confident enough to optimise out the if condition even though the elevel is not changed after it is set from the constant.
Regards
David Rowley
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Re: logical changeset generation v6
От:
Magnus Hagander <magnus@hagander.net>
Дата:
On Mon, Sep 23, 2013 at 7:03 PM, Peter Geoghegan <pg@heroku.com> wrote:
On Mon, Sep 23, 2013 at 9:54 AM, Andres Freund <andres@2ndquadrant.com> wrote:+1
> I still find it wierd/inconsistent to have:
> * pg_receivexlog
> * pg_recvlogical
> binaries, even from the same source directory. Why once "pg_recv" and
> once "pg_receive"?
Digging up a really old thread since I just got annoyed by the inconsistent naming the first time myself :)
I can't find that this discussion actually came to a proper consensus, but I may be missing something. Did we go with pg_recvlogical just because we couldn't decide on a better name, or did we intentionally decide it was the best?
I definitely think pg_receivelogical would be a better name, for consistency (because it's way too late to rename pg_receivexlog of course - once released that can't really chance. Which is why *if* we want to change the name of pg_recvxlog we have a few more days to make a decision..)
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Re: logical changeset generation v6
От:
Magnus Hagander <magnus@hagander.net>
Дата:
On Thu, Apr 24, 2014 at 9:43 AM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2014-04-24 09:39:21 +0200, Magnus Hagander wrote:I went with pg_recvlogical because that's where the (small) majority
> I can't find that this discussion actually came to a proper consensus, but
> I may be missing something. Did we go with pg_recvlogical just because we
> couldn't decide on a better name, or did we intentionally decide it was the
> best?
seemed to be. Even if I was unconvinced. There were so many outstanding
big fights at that point that I didn't want to spend my time on this ;)
I was guessing something like the second part there, which is why I figured this would be a good time to bring this fight back up to the surface ;)
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Re: logical changeset generation v6.7
От:
Fabrízio de Royes Mello <fabriziomello@gmail.com>
Дата:
On Thu, Nov 14, 2013 at 11:46 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>
> On 2013-11-12 18:50:33 +0100, Andres Freund wrote:
> > > You've actually changed the meaning of this section (and not in a good way):
> > >
> > > be set at server start. <varname>wal_level</> must be set
> > > - to <literal>archive</> or <literal>hot_standby</> to allow
> > > - connections from standby servers.
> > > + to <literal>archive</>, <literal>hot_standby</> or <literal>logical</>
> > > + to allow connections from standby servers.
> > >
> > > I think that the previous text meant that you needed archive - or, if
> > > you want to allow connections, hot_standby. The new text loses that
> > > nuance.
> >
> > Yea, that's because it was lost on me in the first place...
>
> I think that's because the nuance isn't actually in the text - note that
> it is talking about max_wal_senders and talking about connections
> *from*, not *to* standby servers.
> I've reformulated the wal_level paragraph and used "or higher" in
> several places now.
>
> Ok, so here's a rebased version of this. I tried to fix all the issues
> you mentioned, and it's based on the split off IsSystemRelation() patch,
> I've sent yesterday (included here).
>
$ gunzip -c /home/fabrizio/Downloads/0002-wal_decoding-Add-wal_level-logical-and-log-data-requ.patch.gz | git apply -
warning: src/backend/access/transam/xlog.c has type 100755, expected 100644
$ gunzip -c /home/fabrizio/Downloads/0005-wal_decoding-Introduce-wal-decoding-via-catalog-time.patch.gz | git apply -
warning: src/backend/access/transam/xlog.c has type 100755, expected 100644
$ gunzip -c /home/fabrizio/Downloads/0006-wal_decoding-Implement-VACUUM-FULL-CLUSTER-support-v.patch.gz | git apply -
warning: src/backend/access/transam/xlog.c has type 100755, expected 100644
$ gunzip -c /home/fabrizio/Downloads/0007-wal_decoding-Only-peg-the-xmin-horizon-for-catalog-t.patch.gz | git apply -
warning: src/backend/access/transam/xlog.c has type 100755, expected 100644
warning: src/backend/access/transam/xlog.c has type 100755, expected 100644
$ gunzip -c /home/fabrizio/Downloads/0005-wal_decoding-Introduce-wal-decoding-via-catalog-time.patch.gz | git apply -
warning: src/backend/access/transam/xlog.c has type 100755, expected 100644
$ gunzip -c /home/fabrizio/Downloads/0006-wal_decoding-Implement-VACUUM-FULL-CLUSTER-support-v.patch.gz | git apply -
warning: src/backend/access/transam/xlog.c has type 100755, expected 100644
$ gunzip -c /home/fabrizio/Downloads/0007-wal_decoding-Only-peg-the-xmin-horizon-for-catalog-t.patch.gz | git apply -
warning: src/backend/access/transam/xlog.c has type 100755, expected 100644
$ gunzip -c /home/fabrizio/Downloads/0011-wal_decoding-pg_recvlogical-Introduce-pg_receivexlog.patch.gz | git apply -
error: patch failed: src/bin/pg_basebackup/streamutil.c:210
error: src/bin/pg_basebackup/streamutil.c: patch does not apply
The others are applied correctly. The permission warning must be fixed and 0011 bust be rebased.
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
