Re: [HACKERS] Allow pg_dumpall to work without pg_authid
On Sun, 19 Feb 2017 at 10:08 Stephen Frost wrote: > If anything, it should use pg_roles, not pg_user. > > I don't really like the "--avoid-pgauthid" option, but "--no-passwords" > would probably work. > > Am sorry, I meant pg_roles (FWIW, the github URL given earlier uses pg_roles). '--no-passwords' is a good alternative. Would submit a patch soon. - Robins -- - robins
Re: [HACKERS] pg_get_object_address() doesn't support composites
On 2/17/17 9:53 PM, Alvaro Herrera wrote:
Jim Nasby wrote:
See below. ISTM that pg_get_object_address should support everything
pg_identify_object_as_address can output, no?
I'm guessing the answer here is to have pg_identify_object_as_address
complain if you ask it for something that's not mapable.
Yes, I think we should just reject the case in
pg_identify_object_as_address.
Attached patch does that, and tests for it. Note that there were some
unsupported types that were not being tested before. I've added a
comment requesting people update the test if they add more types.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
diff --git a/src/backend/catalog/objectaddress.c
b/src/backend/catalog/objectaddress.c
index 2a38792ed6..27ac6ca79a 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -488,7 +488,8 @@ static const ObjectPropertyType ObjectProperty[] =
* do not have corresponding values in the output enum. The user of this map
* must be careful to test for invalid values being returned.
*
- * To ease maintenance, this follows the order of getObjectTypeDescription.
+ * To ease maintenance, this follows the order of getObjectTypeDescription. If
+ * you add anything here please update test/regress/sql/object_address.sql.
*/
static const struct object_type_map
{
@@ -3634,6 +3635,7 @@ pg_identify_object_as_address(PG_FUNCTION_ARGS)
Oid objid = PG_GETARG_OID(1);
int32 subobjid = PG_GETARG_INT32(2);
ObjectAddress address;
+ char *type;
char *identity;
List *names;
List *args;
@@ -3646,6 +3648,13 @@ pg_identify_object_as_address(PG_FUNCTION_ARGS)
address.objectId = objid;
address.objectSubId = subobjid;
+ /* Verify pg_get_object_address() would be able to do something with
this type */
+ type = getObjectTypeDescription(&address);
+ if (read_objtype_from_string(type) < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("unsupported object type \"%s\"",
type)));
+
/*
* Construct a tuple descriptor for the result row. This must match
this
* function's pg_proc entry!
@@ -3661,7 +3670,7 @@ pg_identify_object_as_address(PG_FUNCTION_ARGS)
tupdesc = BlessTupleDesc(tupdesc);
/* object type */
- values[0] = CStringGetTextDatum(getObjectTypeDescription(&address));
+ values[0] = CStringGetTextDatum(type);
nulls[0] = false;
/* object identity */
diff --git a/src/test/regress/expected/object_address.out
b/src/test/regress/expected/object_address.out
index ec5ada97ad..4e99068425 100644
--- a/src/test/regress/expected/object_address.out
+++ b/src/test/regress/expected/object_address.out
@@ -50,8 +50,9 @@ DO $$
DECLARE
objtype text;
BEGIN
- FOR objtype IN VALUES ('toast table'), ('index column'), ('sequence
column'),
- ('toast table column'), ('view column'), ('materialized view
column')
+ FOR objtype IN VALUES ('toast table'), ('composite type'), ('index
column'),
+ ('sequence column'), ('toast table column'), ('view column'),
+ ('materialized view column'), ('composite type column')
LOOP
BEGIN
PERFORM pg_get_object_address(objtype, '{one}', '{}');
@@ -62,11 +63,52 @@ BEGIN
END;
$$;
WARNING: error for toast table: unsupported object type "toast table"
+WARNING: error for composite type: unsupported object type "composite type"
WARNING: error for index column: unsupported object type "index column"
WARNING: error for sequence column: unsupported object type "sequence column"
WARNING: error for toast table column: unsupported object type "toast table
column"
WARNING: error for view column: unsupported object type "view column"
WARNING: error for materialized view column: unsupported object type
"materialized view column"
+WARNING: error for composite type column: unsupported object type "composite
type column"
+DO $$
+DECLARE
+ toastid oid;
+ classid oid;
+ objid oid;
+ objsubid int;
+ objtype text;
+BEGIN
+ SELECT INTO STRICT toastid
+ reltoastrelid
+ FROM pg_class
+ WHERE oid = 'addr_nsp.gentable'::regclass
+ ;
+ FOR classid, objid, objsubid, objtype IN VALUES
+ (1259, toastid, 0, 'toast table'),
+ (1259, 'addr_nsp.gencomptype'::regclass, 0, 'composite type'),
+ (1259, 'addr_nsp.gentable_pkey'::regclass, 1, 'index column'),
+ (1259, 'addr_nsp.gentable_a_seq'::regclass, 1, 'sequence
column'),
+ (1259, to
Re: [HACKERS] Provide list of subscriptions and publications in psql's completion
On Sun, Feb 19, 2017 at 9:50 AM, Michael Paquier wrote: > I have been poking at it, and yeah... I missed the fact that > pg_subcription is not a view. I thought that check_conninfo was being > called in this context only.. Still, storing plain passwords in system catalogs is a practice that should be discouraged as base backup data can go over a network as well... At least adding a note or a warning in the documentation would be nice about the fact that any kind of security-sensitive data should be avoided here. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Provide list of subscriptions and publications in psql's completion
On Sun, Feb 19, 2017 at 8:05 AM, Petr Jelinek wrote: > It's not a view it's system catalog which actually stores the data, how > would it hide anything? I have been poking at it, and yeah... I missed the fact that pg_subcription is not a view. I thought that check_conninfo was being called in this context only.. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add new function dsa_allocate0.
On Sun, Feb 19, 2017 at 12:27 PM, Thomas Munro wrote: > On Sun, Feb 19, 2017 at 11:27 AM, Tom Lane wrote: >> Thomas Munro writes: >>> On Sat, Feb 18, 2017 at 5:41 AM, Tom Lane wrote: Robert Haas writes: > I'm thinking we should change this to look more like the > MemoryContextAlloc interface. >> +1 >> >>> Maybe something like the attached? I didn't add DSA_ALLOC_HUGE >>> because there is currently no limit on allocation size (other than the >>> limit on total size which you can set with dsa_set_size_limit, but >>> that causes allocation failure, not a separate kind of error). Should >>> there be a per-allocation size sanity check of 1GB like palloc? >> >> I think it's not a bad idea. It could help catch faulty allocation >> requests (since I'd bet very few call sites actually intend to allocate >> gigabytes in one go), and as Robert says, there is substantial value in >> the semantics being as much like palloc() as possible. People are >> likely to assume that even if it isn't true. > > Agreed. Here's a patch like that. Oops, that had a typo in a comment. Here's a better one. -- Thomas Munro http://www.enterprisedb.com dsa-extended-v3.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add new function dsa_allocate0.
On Sun, Feb 19, 2017 at 11:27 AM, Tom Lane wrote: > Thomas Munro writes: >> On Sat, Feb 18, 2017 at 5:41 AM, Tom Lane wrote: >>> Robert Haas writes: I'm thinking we should change this to look more like the MemoryContextAlloc interface. > >>> +1 > >> Maybe something like the attached? I didn't add DSA_ALLOC_HUGE >> because there is currently no limit on allocation size (other than the >> limit on total size which you can set with dsa_set_size_limit, but >> that causes allocation failure, not a separate kind of error). Should >> there be a per-allocation size sanity check of 1GB like palloc? > > I think it's not a bad idea. It could help catch faulty allocation > requests (since I'd bet very few call sites actually intend to allocate > gigabytes in one go), and as Robert says, there is substantial value in > the semantics being as much like palloc() as possible. People are > likely to assume that even if it isn't true. Agreed. Here's a patch like that. -- Thomas Munro http://www.enterprisedb.com dsa-extended-v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Allow pg_dumpall to work without pg_authid
Greetings, * Robins Tharakan (thara...@gmail.com) wrote: > I would like to work on a patch to accommodate restricted environments > (such as AWS RDS Postgres) which don't allow pg_authid access since their > definition of Superuser is just a regular user with extra permissions. > > Would you consider a patch to add a flag to work around this restriction, > Or, do you prefer that this be maintained outside core? > > I could add a flag such as --avoid-pgauthid (am open to options) that skips > pg_authid and uses pg_user (but essentially resets all User passwords). > Mostly this is better than not being able to get the dump at all. If anything, it should use pg_roles, not pg_user. I don't really like the "--avoid-pgauthid" option, but "--no-passwords" would probably work. In general, this seems like a reasonable thing to add support for. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] logical replication access control patches
Peter, * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote: > 0002 Add PUBLICATION privilege > > Add a new privilege kind to tables to determine whether they can be > added to a publication. I'm not convinced that it really makes sense to have PUBLICATION of a table be independent from the rights an owner of a table has. We don't allow other ALTER commands on objects based on GRANT'able rights, in general, so I'm not really sure that it makes sense to do so here. The downside of adding these privileges is that we're burning through the last few bits in the ACLMASK for a privilege that doesn't really seem like it's something that would be GRANT'd in general usage. I have similar reservations regarding the proposed SUBSCRIPTION privilege. I'm certainly all for removing the need for users to be the superuser for such commands, just not sure that they should be GRANT'able privileges instead of privileges which the owner of the relation or database has. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Provide list of subscriptions and publications in psql's completion
On 19/02/17 00:02, Michael Paquier wrote: > On Sat, Feb 18, 2017 at 11:57 PM, Petr Jelinek > wrote: >> On 15/02/17 05:56, Michael Paquier wrote: >>> I thought that this was correctly clobbered... But... No that's not >>> the case by looking at the code. And honestly I think that it is >>> unacceptable to show potentially security-sensitive information in >>> system catalogs via a connection string. We are really careful about >>> not showing anything bad in pg_stat_wal_receiver, which also sets to >>> NULL fields for non-superusers and even clobbered values in the >>> printed connection string for superusers, but pg_subscription fails on >>> those points. >>> >> >> I am not following here, pg_subscription is currently superuser only >> catalog, similarly to pg_user_mapping, there is no leaking. > > Even if it is a superuser-only view, pg_subscription does not hide > sensitive values in connection strings while it should. See similar It's not a view it's system catalog which actually stores the data, how would it hide anything? -- Petr Jelinek 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: [HACKERS] Provide list of subscriptions and publications in psql's completion
On Sat, Feb 18, 2017 at 11:57 PM, Petr Jelinek wrote: > On 15/02/17 05:56, Michael Paquier wrote: >> I thought that this was correctly clobbered... But... No that's not >> the case by looking at the code. And honestly I think that it is >> unacceptable to show potentially security-sensitive information in >> system catalogs via a connection string. We are really careful about >> not showing anything bad in pg_stat_wal_receiver, which also sets to >> NULL fields for non-superusers and even clobbered values in the >> printed connection string for superusers, but pg_subscription fails on >> those points. >> > > I am not following here, pg_subscription is currently superuser only > catalog, similarly to pg_user_mapping, there is no leaking. Even if it is a superuser-only view, pg_subscription does not hide sensitive values in connection strings while it should. See similar discussion for pg_stat_wal_receiver here which is also superuser-only (it does display null values for non-superusers): https://www.postgresql.org/message-id/562f6c7f-6a47-0a8a-e189-2de9ea896...@2ndquadrant.com Something needs to be done at least for that, independently on the psql completion. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add new function dsa_allocate0.
Thomas Munro writes: > On Sat, Feb 18, 2017 at 5:41 AM, Tom Lane wrote: >> Robert Haas writes: >>> I'm thinking we should change this to look more like the >>> MemoryContextAlloc interface. >> +1 > Maybe something like the attached? I didn't add DSA_ALLOC_HUGE > because there is currently no limit on allocation size (other than the > limit on total size which you can set with dsa_set_size_limit, but > that causes allocation failure, not a separate kind of error). Should > there be a per-allocation size sanity check of 1GB like palloc? I think it's not a bad idea. It could help catch faulty allocation requests (since I'd bet very few call sites actually intend to allocate gigabytes in one go), and as Robert says, there is substantial value in the semantics being as much like palloc() as possible. People are likely to assume that even if it isn't true. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Keeping pg_recvlogical's "feTimestamp" separate from TimestampTz
I wrote:
> I propose that what we need to do is get rid of the dangerous and
> none-too-readable-anyway use of "int64" to declare replication-protocol
> timestamps, and instead declare them as, say,
> typedef struct RPTimestamp
> {
> int64 rptimestamp;
> } RPTimestamp;
I experimented with this a bit, as in the attached draft patch, and
concluded that use of a struct is probably a bridge too far. It makes the
patch pretty invasive notationally, and yet it still fails to catch places
where nobody had bothered to even think about float timestamps anywhere
nearby; which there are several of, as per my report
https://www.postgresql.org/message-id/26788.1487455...@sss.pgh.pa.us
We might be able to salvage the parts of this that simply redeclare
appropriate variables as "IntegerTimestamp" rather than "int64", with
the type declaration just being "typedef int64 IntegerTimestamp".
If we go forward with keeping protocol fields as integer timestamps
then I'd want to do that, just so we have some notation in the code
as to what the values are meant to be. But right at the moment I'm
thinking we'd be better off to change them all to TimestampTz instead,
and adjust the arithmetic on them appropriately.
regards, tom lane
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index 09ecc15..2f688ec 100644
*** a/src/backend/replication/basebackup.c
--- b/src/backend/replication/basebackup.c
*** static int64 throttling_counter;
*** 95,101
static int64 elapsed_min_unit;
/* The last check of the transfer rate. */
! static int64 throttled_last;
/*
* The contents of these directories are removed or recreated during server
--- 95,101
static int64 elapsed_min_unit;
/* The last check of the transfer rate. */
! static IntegerTimestamp throttled_last;
/*
* The contents of these directories are removed or recreated during server
*** throttle(size_t increment)
*** 1346,1352
return;
/* Time elapsed since the last measurement (and possible wake up). */
! elapsed = GetCurrentIntegerTimestamp() - throttled_last;
/* How much should have elapsed at minimum? */
elapsed_min = elapsed_min_unit * (throttling_counter / throttling_sample);
sleep = elapsed_min - elapsed;
--- 1346,1352
return;
/* Time elapsed since the last measurement (and possible wake up). */
! elapsed = GetCurrentIntegerTimestamp().integer_ts - throttled_last.integer_ts;
/* How much should have elapsed at minimum? */
elapsed_min = elapsed_min_unit * (throttling_counter / throttling_sample);
sleep = elapsed_min - elapsed;
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 0b19fec..30e2f19 100644
*** a/src/backend/replication/logical/worker.c
--- b/src/backend/replication/logical/worker.c
*** ApplyLoop(void)
*** 989,995
start_lsn = pq_getmsgint64(&s);
end_lsn = pq_getmsgint64(&s);
send_time =
! IntegerTimestampToTimestampTz(pq_getmsgint64(&s));
if (last_received < start_lsn)
last_received = start_lsn;
--- 989,995
start_lsn = pq_getmsgint64(&s);
end_lsn = pq_getmsgint64(&s);
send_time =
! Int64ToTimestampTz(pq_getmsgint64(&s));
if (last_received < start_lsn)
last_received = start_lsn;
*** ApplyLoop(void)
*** 1009,1015
endpos = pq_getmsgint64(&s);
timestamp =
! IntegerTimestampToTimestampTz(pq_getmsgint64(&s));
reply_requested = pq_getmsgbyte(&s);
send_feedback(endpos, reply_requested, false);
--- 1009,1015
endpos = pq_getmsgint64(&s);
timestamp =
! Int64ToTimestampTz(pq_getmsgint64(&s));
reply_requested = pq_getmsgbyte(&s);
send_feedback(endpos, reply_requested, false);
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index 5c2e72b..289c806 100644
*** a/src/backend/replication/walreceiver.c
--- b/src/backend/replication/walreceiver.c
*** XLogWalRcvProcessMsg(unsigned char type,
*** 892,898
/* read the fields */
dataStart = pq_getmsgint64(&incoming_message);
walEnd = pq_getmsgint64(&incoming_message);
! sendTime = IntegerTimestampToTimestampTz(
pq_getmsgint64(&incoming_message));
ProcessWalSndrMessage(walEnd, sendTime);
--- 892,898
/* read the fields */
dataStart = pq_getmsgint64(&incoming_message);
walEnd = pq_getmsgint64(&incoming_message);
! sendTime = Int64ToTimestampTz(
pq_getmsgint64(&incoming_message));
ProcessWalSndrMessage(walEnd, sendTime);
*** XLogWalRcvProcessMsg(unsigned char type,
*** 913,919
/* read the fields */
walEnd = pq_getmsgint64(&incoming_message);
! sendTime = IntegerTimestampToTimestam
[HACKERS] Replication vs. float timestamps is a disaster
Both the streaming replication and logical replication areas of the code are, approximately, utterly broken when !HAVE_INT64_TIMESTAMPS. (The fact that "make check-world" passes anyway is an indictment of the quality of the regression tests.) I started poking around in this area after Thomas Munro reported that pg_recvlogical.c no longer even compiles with --disable-integer-datetimes, but the problems go much deeper than that. replication/logical/worker.c's send_feedback() is sending a backend timestamp directly as sendTime. This is wrong per the protocol spec, which says the field is an integer timestamp. I'm not sure if it's OK to just replace -pq_sendint64(reply_message, now);/* sendTime */ +pq_sendint64(reply_message, GetCurrentIntegerTimestamp()); /* sendTime */ ... is it important for the sent timestamp to exactly match the timestamp that was used earlier in the function for deciding whether to send or not? Should we instead try to convert the earlier logic to use integer timestamps? As for logical replication, logicalrep_write_begin and logicalrep_write_commit are likewise sending TimestampTz values using pq_sendint64, and this is much harder to patch locally since the values were taken from WAL records that contain TimestampTz. Although the sent values are nonsensical according to the protocol spec, logicalrep_read_begin and logicalrep_read_commit read them with pq_getmsgint64 and assign the results back to TimestampTz, which means that things sort of appear to work as long as you don't mind losing all sub-second precision in the timestamps. (But a decoder that was written to spec would think the values are garbage.) I'm inclined to think that at least for logical replication, we're going to have to give up the protocol spec wording that says that timestamps are integers, and instead admit that they are dependent on disable-integer-datetimes. Maybe we should do the same in the streaming replication protocol, because this area is going to be a very fertile source of bugs for the foreseeable future if we don't. It's not like replication between enable-integer-datetimes and disable-integer-datetimes hosts has any chance of being useful anyway. Thoughts? Should we double down on trying to make this work according to the "all integer timestamps" protocol specs, or cut our losses and change the specs? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add new function dsa_allocate0.
On Sat, Feb 18, 2017 at 5:41 AM, Tom Lane wrote: > Robert Haas writes: >> I'm thinking we should change this to look more like the >> MemoryContextAlloc interface. Let's have DSA_ALLOC_HUGE, >> DSA_ALLOC_NO_OOM, and DSA_ALLOC_ZERO, just like the corresponding >> MCXT_* flags, and a function dsa_allocate_extended() that takes a >> flags argument. Then, dsa_allocate(x,y) can be a macro for >> dsa_allocate_extended(x,y,0) and dsa_allocate0(x,y) can be a macro for >> dsa_allocate_extended(x,y,DSA_ALLOC_ZERO). What this goof on my (and >> Dilip's) part illustrates to me is that having this interface behave >> significantly differently from the MemoryContextAlloc interface is >> going to cause mistakes. > > +1 Maybe something like the attached? I didn't add DSA_ALLOC_HUGE because there is currently no limit on allocation size (other than the limit on total size which you can set with dsa_set_size_limit, but that causes allocation failure, not a separate kind of error). Should there be a per-allocation size sanity check of 1GB like palloc? -- Thomas Munro http://www.enterprisedb.com dsa-extended.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Allow pg_dumpall to work without pg_authid
Hi, I would like to work on a patch to accommodate restricted environments (such as AWS RDS Postgres) which don't allow pg_authid access since their definition of Superuser is just a regular user with extra permissions. Would you consider a patch to add a flag to work around this restriction, Or, do you prefer that this be maintained outside core? I could add a flag such as --avoid-pgauthid (am open to options) that skips pg_authid and uses pg_user (but essentially resets all User passwords). Mostly this is better than not being able to get the dump at all. I have a fork here (a few weeks old): https://github.com/postgres/postgres/commit/552f4d92fde9518f934447e032b8ffcc945f12d9 Thanks Robins Tharakan http://www.thatguyfromdelhi.com/2016/12/custom-pgdumpall-now-works-with-aws.html -- - robins
Re: [HACKERS] Instability in select_parallel regression test
Robert Haas writes: > On Fri, Feb 17, 2017 at 9:57 PM, Amit Kapila wrote: >>> That seems like a seriously broken design to me, first because it can make >>> for a significant delay in the slots becoming available (which is what's >>> evidently causing these regression failures), and second because it's >>> simply bad design to load extra responsibilities onto the postmaster. >>> Especially ones that involve touching shared memory. > It's a little surprising to me that the delay we're seeing here is > significant, because the death of a child should cause an immediate > SIGCHLD, resulting in a call to reaper(), resulting in a call to > waitpid(). Why's that not working? I can't say for sure about gharial, but gaur/pademelon is a single-CPU machine, and I have reason to believe that its scheduler discriminates pretty hard against processes that have been deemed to be background processes. The reason it's not working is simply that the postmaster doesn't get a chance to run until the active backend has already decided that there are no free slots for the next parallel query (indeed, the next several parallel queries). Yeah, it got the signal, and eventually it gets some actual cycles, but not soon enough. On a sufficiently loaded machine this could be expected to happen, at least occasionally, even if you had lots of CPUs. It's merely more readily reproducible on this particular configuration. Also, you're assuming that the parallel worker process gets enough more cycles to exit once it's woken the parent backend with an "I'm done" signal. I wouldn't care to bet on that happening promptly either. I have definitely seen HPUX do a process context swap *immediately* when it sees a lower-priority process signal a higher-priority one, and not give the first process any more cycles for a good long time. >> There seem to be many reasons why exit of background workers is done >> by postmaster like when they have to restart after a crash or if >> someone terminates them (TerminateBackgroundWorker()) or if the >> notification has to be sent at exit (bgw_notify_pid). Moreover, there >> can be background workers without shared memory access as well which >> can't clear state from shared memory. Another point to think is that >> background workers contain user-supplied code, so not sure how good it >> is to give them control of tinkering with shared data structures of >> the backend. Now, one can argue that for some of the cases where it >> is possible background worker should cleanup shared memory state at >> the exit, but I think it is not directly related to the parallel >> query. As far as the parallel query is concerned, it just needs to >> wait for workers exit to ensure that no more operations can be >> performed in workers after that point so that it can accumulate stats >> and retrieve some fixed parallel state information. > That's a pretty good list of reasons with which I agree. It seems largely bogus IMO, except possibly the "no access to shared memory" reason, which is irrelevant for the problem at hand; I see no very probable reason why backends would ever be interested in launching workers that they couldn't communicate with. In particular, I'll buy the "user-supplied code" one only if we rip out every possibility of executing user-supplied C or plperlu or plpythonu code in standard backends. There is *no* good reason for treating parallel workers as less reliable than standard backends; if anything, given the constraints on what we let them do, they should be more reliable. Basically, I think we need to fix things so that WaitForParallelWorkersToFinish doesn't return until the slot is free and there are no impediments to launching another worker. Anything less is going to result in race conditions and intermittent misbehavior on heavily-loaded machines. Personally I think that it'd be a good idea to get the postmaster out of the loop while we're at it, but the minimum change is to wait for the slot to be marked free by the postmaster. I think what we need to do here is to move the test that needs workers to execute before other parallel query tests where there is no such requirement. >>> That's not fixing the problem, it's merely averting your eyes from >>> the symptom. >> I am not sure why you think so. In other words, you are willing to accept a system in which we can never be sure that any query after the first one in select_parallel actually ran in parallel? That seems silly on its face, and an enormous restriction on what we'll actually be able to test. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] new gcc 7.0.1 warnings
2017-02-18 18:35 GMT+01:00 Tom Lane :
> Pavel Stehule writes:
> > float.c:382:5: note: ‘snprintf’ output between 2 and 311 bytes into a
> > destination of size 65
> > float.c:618:5: note: ‘snprintf’ output between 2 and 311 bytes into a
> > destination of size 129
>
> That's kind of annoying. I suppose the point is that the compiler can't
> see what precision we're selecting, and with sufficiently large precision
> the output could be that wide. But actually the precision should be small
> enough to make that OK.
>
> Do the warnings go away if you add some explicit guard to the precision
> variable, say like this:
>
> {
> intndig = DBL_DIG + extra_float_digits;
>
> if (ndig < 1)
> ndig = 1;
> + if (ndig > 50)
> + ndig = 50;
>
This fix doesn't help
Regards
Pavel
>
> snprintf(ascii, MAXDOUBLEWIDTH + 1, "%.*g", ndig, num);
> }
>
> If not, I guess we could increase the size of the palloc'd strings,
> but that seems wasteful.
>
> regards, tom lane
>
Re: [HACKERS] new gcc 7.0.1 warnings
Pavel Stehule writes:
> float.c:382:5: note: ‘snprintf’ output between 2 and 311 bytes into a
> destination of size 65
> float.c:618:5: note: ‘snprintf’ output between 2 and 311 bytes into a
> destination of size 129
That's kind of annoying. I suppose the point is that the compiler can't
see what precision we're selecting, and with sufficiently large precision
the output could be that wide. But actually the precision should be small
enough to make that OK.
Do the warnings go away if you add some explicit guard to the precision
variable, say like this:
{
intndig = DBL_DIG + extra_float_digits;
if (ndig < 1)
ndig = 1;
+ if (ndig > 50)
+ ndig = 50;
snprintf(ascii, MAXDOUBLEWIDTH + 1, "%.*g", ndig, num);
}
If not, I guess we could increase the size of the palloc'd strings,
but that seems wasteful.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Instability in select_parallel regression test
On Fri, Feb 17, 2017 at 9:57 PM, Amit Kapila wrote: >> That seems like a seriously broken design to me, first because it can make >> for a significant delay in the slots becoming available (which is what's >> evidently causing these regression failures), and second because it's >> simply bad design to load extra responsibilities onto the postmaster. >> Especially ones that involve touching shared memory. It's a little surprising to me that the delay we're seeing here is significant, because the death of a child should cause an immediate SIGCHLD, resulting in a call to reaper(), resulting in a call to waitpid(). Why's that not working? >> I think this needs to be changed, and promptly. Why in the world don't >> you simply have the workers clearing their slots when they exit? > > There seem to be many reasons why exit of background workers is done > by postmaster like when they have to restart after a crash or if > someone terminates them (TerminateBackgroundWorker()) or if the > notification has to be sent at exit (bgw_notify_pid). Moreover, there > can be background workers without shared memory access as well which > can't clear state from shared memory. Another point to think is that > background workers contain user-supplied code, so not sure how good it > is to give them control of tinkering with shared data structures of > the backend. Now, one can argue that for some of the cases where it > is possible background worker should cleanup shared memory state at > the exit, but I think it is not directly related to the parallel > query. As far as the parallel query is concerned, it just needs to > wait for workers exit to ensure that no more operations can be > performed in workers after that point so that it can accumulate stats > and retrieve some fixed parallel state information. That's a pretty good list of reasons with which I agree. To underscore one of Amit's points, bgw_notify_pid requires SIGUSR1 to be sent to the process that registered the worker if that process is still running, both when the process is started and when it terminates. The workers have no way of keeping track of whether the process that did the registration is still running, and with bad luck might SIGUSR1 an unrelated process (possibly killing it). And those SIGUSR1s are absolutely necessary, because otherwise a process that is waiting for a worker to start or stop would have to poll, which would make the whole system more resource-intensive and a whole lot less responsive. There are a few more reasons Amit didn't mention: 1. Workers can fail during startup before they can possibly have done enough work to be able to mark their slots as being free. On Linux, fork() can fail; on Windows, you can fail either to start a new process or to have it reattach to the shared memory segment. You could try to have the postmaster catch just those cases where the worker doesn't get far enough and have the worker do the others, but that's almost bound to make things more complex and fragile. I can't see us coming out ahead there. 2. Worker slots don't get freed when the process terminates; they get freed when the background worker is not running and will never again be restarted. So there's a subtle lifespan difference here: a background worker slot is consumed before any process is started, and persists across possibly many restarts of that process, and is finally freed when the process is both not currently running and not ever going to be restarted. It would no doubt be a bit fragile if the worker tried to guess whether or not the postmaster was going to restart it and free the slot only if not -- what happens if the postmaster comes to a different conclusion? >>> I think what we need to do >>> here is to move the test that needs workers to execute before other >>> parallel query tests where there is no such requirement. >> >> That's not fixing the problem, it's merely averting your eyes from >> the symptom. >> > I am not sure why you think so. Parallel query is capable of running > without workers even if the number of planned workers are not > available and there are many reasons for same. In general, I think > the tests should not rely on the availability of background workers > and if there is a test like that then it should be the responsibility > of the test to ensure the same. I agree with that, too. One thing that I'm a little concerned about, though, is that users could also see the kind of behavior Tom seems to be describing here and might find it surprising. For example, suppose your queries all use 4 workers and you have 4 workers configured. If you're the only user on the system and you run query after query after query, you expect that all of those queries are going to get all 4 workers. If the postmaster is so unresponsive that you don't get all of them, or worse you don't get any, you might be tempted to swear at the stupid PostgreSQL developer who engineered this system. (Hi, swearing people!
Re: [HACKERS] Parallel bitmap heap scan
On Fri, Feb 17, 2017 at 2:01 AM, Robert Haas wrote:
> + * in order to share relptrs of the chunk and the pages arrays and other
> + * TBM related information with other workers.
>
> No relptrs any more.
Fixed
>
> +dsa_pointer dsapagetable;/* dsa_pointer to the element array */
> +dsa_pointer dsapagetableold;/* dsa_pointer to the old element array
> */
> +dsa_area *dsa;/* reference to per-query dsa area */
> +dsa_pointer ptpages;/* dsa_pointer to the page array */
> +dsa_pointer ptchunks;/* dsa_pointer to the chunk array */
>
> Let's put the DSA pointer first and then the other stuff after it.
> That seems more logical.
Done that way
>
> +typedef struct TBMSharedIteratorState
> +{
> +intspageptr;/* next spages index */
> +intschunkptr;/* next schunks index */
> +intschunkbit;/* next bit to check in current schunk
> */
> +LWLocklock;/* lock to protect above members */
> +dsa_pointer pagetable;/* dsa pointers to head of pagetable data
> */
> +dsa_pointer spages;/* dsa pointer to page array */
> +dsa_pointer schunks;/* dsa pointer to chunk array */
> +intnentries;/* number of entries in pagetable */
> +intmaxentries;/* limit on same to meet maxbytes */
> +intnpages;/* number of exact entries in
> pagetable */
> +intnchunks;/* number of lossy entries in pagetable */
> +} TBMSharedIteratorState;
>
> I think you've got this largely backwards from the most logical order.
> Let's order it like this: nentries, maxentries, npages, nchunks,
> pagetable, spages, schunks, lock (to protect BELOW members), spageptr,
> chunkptr, schunkbit.
Done
>
> +struct TBMSharedIterator
> +{
> +PagetableEntry *ptbase;/* pointer to the pagetable element array
> */
> +dsa_area *dsa;/* reference to per-query dsa area */
> +int *ptpages;/* sorted exact page index list */
> +int *ptchunks;/* sorted lossy page index list */
> +TBMSharedIteratorState *state;/* shared members */
> +TBMIterateResult output;/* MUST BE LAST (because variable-size) */
> +};
>
> Do we really need "dsa" here when it's already present in the shared
> state? It doesn't seem like we even use it for anything. It's needed
> to create each backend's TBMSharedIterator, but not afterwards, I
> think.
Right, removed.
>
> In terms of ordering, I'd move "state" to the top of the structure,
> just like "tbm" comes first in a TBMIterator.
Yeah, that looks better. Done that way.
>
> + * total memory consumption. If dsa passed to this function is not NULL
> + * then the memory for storing elements of the underlying page table will
> + * be allocated from the DSA.
>
> Notice that you capitalized "DSA" in two different ways in the same
> sentence; I'd go for the all-caps version. Also, you need the word
> "the" before the first one.
Fixed, all such instances.
>
> +if (tbm->status == TBM_HASH && (tbm->iterating == TBM_NOT_ITERATING))
>
> Excess parentheses.
Fixed
>
> + * tbm_prepare_shared_iterate - prepare to iterator through a TIDBitmap
> + * by multiple workers using shared iterator.
>
> Prepare to iterate, not prepare to iterator. I suggest rephrasing
> this as "prepare shared iteration state for a TIDBitmap".
Fixed.
>
> + * The TBMSharedIteratorState will be allocated from DSA so that multiple
> + * worker can attach to it and iterate jointly.
>
> Maybe change to "The necessary shared state will be allocated from the
> DSA passed to tbm_create, so that multiple workers can attach to it
> and iterate jointly".
Done.
>
> + * This will convert the pagetable hash into page and chunk array of the
> index
> + * into pagetable array so that multiple workers can use it to get the actual
> + * page entry.
>
> I think you can leave off everything starting from "so that". It's
> basically redundant with what you already said.
Done
>
> +dsa_pointer iterator;
> +TBMSharedIteratorState *iterator_state;
>
> These aren't really great variable names, because the latter isn't the
> state associated with the former. They're both the same object.
> Maybe just "dp" and "istate".
Done
>
> I think this function should Assert(tbm->dsa != NULL) and
> Assert(tbm->iterating != TBM_ITERATING_PRIVATE), and similarly
> tbm_begin_iterate should Assert(tbm->iterating != TBM_ITERATE_SHARED).
Done
>
> +/*
> + * If we have a hashtable, create and fill the sorted page lists, unless
> + * we already did that for a previous iterator. Note that the lists are
> + * attached to the bitmap not the iterator, so they can be used by more
> + * than one iterator. However, we keep dsa_pointer to these in the
> shared
> + * iterator so that other workers can access them directly.
>
[HACKERS] Suppressing that pesky warning with older flex versions
While experimenting with enabling -Werror in the buildfarm, I got
annoyed about the fact that we have to apply -Wno-error while
building some of the flex scanners, because with flex versions
before 2.5.36 you get
scan.c: In function 'yy_try_NUL_trans':
scan.c:10317: warning: unused variable 'yyg'
psqlscan.c: In function 'yy_try_NUL_trans':
psqlscan.c:4524: warning: unused variable 'yyg'
psqlscanslash.c: In function 'yy_try_NUL_trans':
psqlscanslash.c:1886: warning: unused variable 'yyg'
I spent some time researching whether there's a way to get rid of that.
It appears that the contents of the yy_try_NUL_trans() function are
fixed depending on the flex options you select, and we don't really want
to change our option choices. So getting these older flex versions to
emit non-broken code seems out of reach. However, there's exactly one
instance of the problematic code, and it's even helpfully labeled:
{
register int yy_is_jam;
struct yyguts_t * yyg = (struct yyguts_t*)yyscanner; /* This var may be
unused depending upon options. */
register int yy_c = 256;
register yyconst struct yy_trans_info *yy_trans_info;
yy_trans_info = &yy_current_state[(unsigned int) yy_c];
yy_current_state += yy_trans_info->yy_nxt;
yy_is_jam = (yy_trans_info->yy_verify != yy_c);
return yy_is_jam ? 0 : yy_current_state;
}
It seems like it would be quite simple and reliable to apply a patch
that inserts "(void) yyg;" into this function. (Which, indeed, is
essentially how flex 2.5.36 and later fixed it.)
I think we could mechanize this as a small perl script that gets invoked
immediately after running flex proper. That way, the fix would already
be applied in "scan.c" and friends in any shipped tarball, and we'd not
be creating any more build-time perl dependency than we already have.
Obviously, this is pretty ugly. But having to carry -Wno-error on the
scanners for the foreseeable future is no pleasing prospect either.
Thoughts?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] "SQL sentence"?
On 2/17/17 10:46 PM, Alvaro Herrera wrote: Sure. We have the extension that turned the command into JSON. It's still an unfinished patch, sadly, even though Alex Shulgin spent a lot of effort trying to get it finished. It is still missing a nontrivial amount of work, but within reach ISTM. You're speaking of https://www.postgresql.org/message-id/CACACo5Q_UXYwF117LBhjZ3xaMPyrgqnqE%3DmXvRhEfjJ51aCfwQ%40mail.gmail.com ? Can you reply to that to restart discussion? -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Provide list of subscriptions and publications in psql's completion
On 15/02/17 05:56, Michael Paquier wrote: > On Wed, Feb 15, 2017 at 3:18 AM, Jim Nasby wrote: >> Why not do what we do for pg_stat_activity.current_query and leave it NULL >> for non-SU? > > If subcriptions are designed to be superuser-only, it seems fair to me > to do so. Some other system SRFs do that already. > >> Even better would be if we could simply strip out password info. Presumably >> we already know how to parse the contents, so I'd think that shouldn't be >> that difficult. > > I thought that this was correctly clobbered... But... No that's not > the case by looking at the code. And honestly I think that it is > unacceptable to show potentially security-sensitive information in > system catalogs via a connection string. We are really careful about > not showing anything bad in pg_stat_wal_receiver, which also sets to > NULL fields for non-superusers and even clobbered values in the > printed connection string for superusers, but pg_subscription fails on > those points. > I am not following here, pg_subscription is currently superuser only catalog, similarly to pg_user_mapping, there is no leaking. -- Petr Jelinek 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: [pgsql-www] [HACKERS] Small issue in online devel documentation build
Hello Magnus, It turns out the "c2" class is added by tidy. The reason is this: http://api.html-tidy.org/tidy/quickref_5.0.0.html#clean I've removed the flag for the devel docs build for now (or - for any XML based docs build). I've also forced another docs load, so the results can be checked. Indeed, thanks, now it looks great... under firefox at least. Another issue in the new HTML documentation, this time not related to the web site. Under chrome there are some strange font size effects on options, for instance with this HTML in "app-psql.html": -f filename For previous versions, the following html was generated: -f filename The filename in the newer html appears much larger under chrome, seemingly because of the within a . Maybe a bug in chrome CSS interpretation, because CSS on code seems to indicate "font-size: 1.3em", but it seems to do 1.3**2 instead for "filename"... However it does not do that elsewhere so it may not be that simple... Basically it looks ok under chrome if in the initial sgml file there is: -s scale_factor (replaceable is out of option) But bad on: --tablespace=tablespace (replaceable is in the option), because the size change is somehow applied twice. The docbook doc says that "replaceable" is a children of "option", which seems to suggest that this nested usage is legit... but it may be buggy as well. This may be fixed/worked around in several places: 1. html generation could be clever enough not to nest "code" tags? or generate tt tags instead as was done before? I have not found where this transformation is defined, probably somewhere within "docbook"... 2. avoid replaceable within option in initial sgml files => many changes, will reappear if someone forgets. 3. some more post-processing tidying could be done hmmm:-( 4. chrome should be repaired if there is indeed a bug... -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Logical replication existing data copy
On 18/02/17 14:17, Erik Rijkers wrote: > On 2017-02-16 00:43, Petr Jelinek wrote: >> On 13/02/17 14:51, Erik Rijkers wrote: >>> On 2017-02-11 11:16, Erik Rijkers wrote: On 2017-02-08 23:25, Petr Jelinek wrote: > 0001-Use-asynchronous-connect-API-in-libpqwalreceiver-v2.patch > 0002-Always-initialize-stringinfo-buffers-in-walsender-v2.patch > 0003-Fix-after-trigger-execution-in-logical-replication-v2.patch > 0004-Add-RENAME-support-for-PUBLICATIONs-and-SUBSCRIPTION-v2.patch > 0001-Logical-replication-support-for-initial-data-copy-v4.patch This often works but it also fails far too often (in my hands). I >> >> That being said, I am so far having problems reproducing this on my test >> machine(s) so no idea what causes it yet. >> > > A few extra bits: > > - I have repeated this now on three different machines (debian 7, 8, > centos6; one a pretty big server); there is always failure within a few > tries of that test program (i.e. pgbench_derail2.sh, with the above 5 > patches). > > - I have also tried to go back to an older version of logrep: running > with 2 instances with only the first four patches (i.e., leaving out the > support-for-existing-data patch). With only those 4, the logical > replication is solid. (a quick 25x repetition of a (very similar) test > program is 100% successful). So the problem is likely somehow in that > last 5th patch. > > - A 25x repetition of a test on a master + replica 5-patch server yields > 13 ok, 12 NOK. > > - Is the 'make check' FAILED test 'object_addess' unrelated? (Can you > at least reproduce that failed test?) > Yes, it has nothing to do with that, that just needs to be updated to use SKIP CONNECT instead of NOCREATE SLOT in this patch since NOCREATE SLOT is no longer enough to skip the connection attempt. And I have that fixed locally, but that does not deserve new patch version given the main issue you reported. -- Petr Jelinek 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: [HACKERS] Logical replication existing data copy
On 18/02/17 14:24, Erik Rijkers wrote: >> >> Maybe add this to the 10 Open Items list? >> https://wiki.postgresql.org/wiki/PostgreSQL_10_Open_Items >> >> It may garner a bit more attention. >> > > Ah sorry, it's there already. Hmm that's interesting given that it's not even committed yet :) -- Petr Jelinek 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: [HACKERS] Logical replication existing data copy
On 2017-02-11 11:16, Erik Rijkers wrote:
On 2017-02-08 23:25, Petr Jelinek wrote:
0001-Use-asynchronous-connect-API-in-libpqwalreceiver-v2.patch
0002-Always-initialize-stringinfo-buffers-in-walsender-v2.patch
0003-Fix-after-trigger-execution-in-logical-replication-v2.patch
0004-Add-RENAME-support-for-PUBLICATIONs-and-SUBSCRIPTION-v2.patch
0001-Logical-replication-support-for-initial-data-copy-v4.patch
Let me add the script ('instances.sh') that I use to startup the two
logical replication instances for testing.
Together with the earlier posted 'pgbench_derail2.sh' it makes out the
fails test.
pg_config of the master is:
$ pg_config
BINDIR =
/home/aardvark/pg_stuff/pg_installations/pgsql.logical_replication/bin
DOCDIR =
/home/aardvark/pg_stuff/pg_installations/pgsql.logical_replication/share/doc
HTMLDIR =
/home/aardvark/pg_stuff/pg_installations/pgsql.logical_replication/share/doc
INCLUDEDIR =
/home/aardvark/pg_stuff/pg_installations/pgsql.logical_replication/include
PKGINCLUDEDIR =
/home/aardvark/pg_stuff/pg_installations/pgsql.logical_replication/include
INCLUDEDIR-SERVER =
/home/aardvark/pg_stuff/pg_installations/pgsql.logical_replication/include/server
LIBDIR =
/home/aardvark/pg_stuff/pg_installations/pgsql.logical_replication/lib
PKGLIBDIR =
/home/aardvark/pg_stuff/pg_installations/pgsql.logical_replication/lib
LOCALEDIR =
/home/aardvark/pg_stuff/pg_installations/pgsql.logical_replication/share/locale
MANDIR =
/home/aardvark/pg_stuff/pg_installations/pgsql.logical_replication/share/man
SHAREDIR =
/home/aardvark/pg_stuff/pg_installations/pgsql.logical_replication/share
SYSCONFDIR =
/home/aardvark/pg_stuff/pg_installations/pgsql.logical_replication/etc
PGXS =
/home/aardvark/pg_stuff/pg_installations/pgsql.logical_replication/lib/pgxs/src/makefiles/pgxs.mk
CONFIGURE =
'--prefix=/home/aardvark/pg_stuff/pg_installations/pgsql.logical_replication'
'--bindir=/home/aardvark/pg_stuff/pg_installations/pgsql.logical_replication/bin'
'--libdir=/home/aardvark/pg_stuff/pg_installations/pgsql.logical_replication/lib'
'--with-pgport=6972' '--enable-depend' '--enable-cassert'
'--enable-debug' '--with-openssl' '--with-perl' '--with-libxml'
'--with-libxslt' '--with-zlib' '--enable-tap-tests'
'--with-extra-version=_logical_replication_20170218_1221_e3a58c8835a2'
CC = gcc
CPPFLAGS = -DFRONTEND -D_GNU_SOURCE -I/usr/include/libxml2
CFLAGS = -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv
-fexcess-precision=standard -g -O2
CFLAGS_SL = -fpic
LDFLAGS = -L../../src/common -Wl,--as-needed
-Wl,-rpath,'/home/aardvark/pg_stuff/pg_installations/pgsql.logical_replication/lib',--enable-new-dtags
LDFLAGS_EX =
LDFLAGS_SL =
LIBS = -lpgcommon -lpgport -lxslt -lxml2 -lssl -lcrypto -lz -lreadline
-lrt -lcrypt -ldl -lm
VERSION = PostgreSQL
10devel_logical_replication_20170218_1221_e3a58c8835a2
I hope it helps someone to reproduce the errors I get. (If you don't,
I'd like to hear that too)
thanks,
Erik Rijkers
#!/bin/sh
port1=6972
port2=6973
project1=logical_replication
project2=logical_replication2
pg_stuff_dir=$HOME/pg_stuff
PATH1=$pg_stuff_dir/pg_installations/pgsql.$project1/bin:$PATH
PATH2=$pg_stuff_dir/pg_installations/pgsql.$project2/bin:$PATH
server_dir1=$pg_stuff_dir/pg_installations/pgsql.$project1
server_dir2=$pg_stuff_dir/pg_installations/pgsql.$project2
data_dir1=$server_dir1/data
data_dir2=$server_dir2/data
options1="
-c wal_level=logical
-c max_replication_slots=10
-c max_worker_processes=12
-c max_logical_replication_workers=10
-c max_wal_senders=14
-c logging_collector=on
-c log_directory=$server_dir1
-c log_filename=logfile.${project1} "
options2="
-c wal_level=replica
-c max_replication_slots=10
-c max_worker_processes=12
-c max_logical_replication_workers=10
-c max_wal_senders=14
-c logging_collector=on
-c log_directory=$server_dir2
-c log_filename=logfile.${project2} "
export PATH=$PATH1; which postgres; postgres -D $data_dir1 -p $port1 ${options1} &
export PATH=$PATH2; which postgres; postgres -D $data_dir2 -p $port2 ${options2} &
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Logical replication existing data copy
Maybe add this to the 10 Open Items list? https://wiki.postgresql.org/wiki/PostgreSQL_10_Open_Items It may garner a bit more attention. Ah sorry, it's there already. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Logical replication existing data copy
On 2017-02-16 00:43, Petr Jelinek wrote: On 13/02/17 14:51, Erik Rijkers wrote: On 2017-02-11 11:16, Erik Rijkers wrote: On 2017-02-08 23:25, Petr Jelinek wrote: 0001-Use-asynchronous-connect-API-in-libpqwalreceiver-v2.patch 0002-Always-initialize-stringinfo-buffers-in-walsender-v2.patch 0003-Fix-after-trigger-execution-in-logical-replication-v2.patch 0004-Add-RENAME-support-for-PUBLICATIONs-and-SUBSCRIPTION-v2.patch 0001-Logical-replication-support-for-initial-data-copy-v4.patch This often works but it also fails far too often (in my hands). I That being said, I am so far having problems reproducing this on my test machine(s) so no idea what causes it yet. A few extra bits: - I have repeated this now on three different machines (debian 7, 8, centos6; one a pretty big server); there is always failure within a few tries of that test program (i.e. pgbench_derail2.sh, with the above 5 patches). - I have also tried to go back to an older version of logrep: running with 2 instances with only the first four patches (i.e., leaving out the support-for-existing-data patch). With only those 4, the logical replication is solid. (a quick 25x repetition of a (very similar) test program is 100% successful). So the problem is likely somehow in that last 5th patch. - A 25x repetition of a test on a master + replica 5-patch server yields 13 ok, 12 NOK. - Is the 'make check' FAILED test 'object_addess' unrelated? (Can you at least reproduce that failed test?) Maybe add this to the 10 Open Items list? https://wiki.postgresql.org/wiki/PostgreSQL_10_Open_Items It may garner a bit more attention. thanks, Erik Rijkers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Gather Merge
On Fri, Feb 17, 2017 at 6:27 PM, Rushabh Lathia
wrote:
> On Fri, Feb 17, 2017 at 4:47 PM, Amit Kapila
> wrote:
>>
>> Are you suggesting that commit 0c2070ce has helped to improve
>> performance if so, I don't think that has been proved? I guess the
>> numbers are different either due to different m/c or some other
>> settings like scale factor or work_mem.
>
>
> I don't really think 0c2070ce is the exact reason. I run the tpch runs
> with the same same setting as what Robert was running. I haven't
> noticed any regression with the runs. For the last runs I also
> uploaded the tpch run outputs for the individual queries for the
> reference.
>
Okay, then I am not sure why you and Robert are seeing different
results, probably because you are using a different machine. Another
thing which we might need to think about this patch is support of
mark/restore position. As of now the paths which create data in
sorted order like sort node and indexscan have support for
mark/restore position. This is required for correctness when such a
node appears on the inner side of Merge Join. Even though this patch
doesn't support mark/restore, it will not produce wrong results
because planner inserts Materialize node to compensate for it, refer
below code.
final_cost_mergejoin()
{
..
/*
* Even if materializing doesn't look cheaper, we *must* do it if the
* inner path is to be used directly (without sorting) and it doesn't
* support mark/restore.
*
* Since the inner side must be ordered, and only Sorts and IndexScans can
* create order to begin with, and they both support mark/restore, you
* might think there's no problem --- but you'd be wrong. Nestloop and
* merge joins can *preserve* the order of their inputs, so they can be
* selected as the input of a mergejoin, and they don't support
* mark/restore at present.
*
* We don't test the value of enable_material here, because
* materialization is required for correctness in this case, and turning
* it off does not entitle us to deliver an invalid plan.
*/
else if (innersortkeys == NIL &&
!ExecSupportsMarkRestore(inner_path))
path->materialize_inner = true;
..
}
I think there is a value in supporting mark/restore position for any
node which produces sorted results, however, if you don't want to
support it, then I think we should update above comment in the code to
note this fact. Also, you might want to check other places to see if
any similar comment updates are required in case you don't want to
support mark/restore position for GatherMerge.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Help text for pg_basebackup -R
On Fri, Feb 17, 2017 at 5:21 PM, Tom Lane wrote: > Stephen Frost writes: > > * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > >> Magnus Hagander wrote: > >>> I'm guessing if we backpatch something like that, it would cause > issues for > >>> translations, right? So we should make it head only? > > >> We've had the argument a number of times. My stand is that many > >> translators are active in the older branches, so this update would be > >> caught there too; and even if not, an updated English message is better > >> than an outdated native-language message. > > > That makes sense to me, at least, so +1, for my part. > > Yeah, if the existing message text is actually wrong or misleading, > we should back-patch. I'm not sure I would do that if it's just a > cosmetic improvement. In this particular case, +1. > OK. Applied and backpatched. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
[HACKERS] [PATCH] Incremental sort (was: PoC: Partial sort)
Hi all! I decided to start new thread for this patch for following two reasons. * It's renamed from "Partial sort" to "Incremental sort" per suggestion by Robert Haas [1]. New name much better characterizes the essence of algorithm. * I think it's not PoC anymore. Patch received several rounds of review and now it's in the pretty good shape. Attached revision of patch has following changes. * According to review [1], two new path and plan nodes are responsible for incremental sort: IncSortPath and IncSort which are inherited from SortPath and Sort correspondingly. That allowed to get rid of set of hacks with minimal code changes. * According to review [1] and comment [2], previous tuple is stored in standalone tuple slot of SortState rather than just HeapTuple. * New GUC parameter enable_incsort is introduced to control planner ability to choose incremental sort. * Test of postgres_fdw with not pushed down cross join is corrected. It appeared that with incremental sort such query is profitable to push down. I changed ORDER BY columns so that index couldn't be used. I think this solution is more elegant than setting enable_incsort = off. Also patch has set of assorted code and comments improvements. Links 1. https://www.postgresql.org/message-id/CA+TgmoZapyHRm7NVyuyZ+yAV=u1a070bogre7pkgyraegr4...@mail.gmail.com 2. https://www.postgresql.org/message-id/cam3swzql4yd2sndhemcgl0q2b2otdkuvv_l6zg_fcgoluwm...@mail.gmail.com -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company incremental-sort-1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
