[HACKERS] Fwd: Patch for reserved connections for replication users
Sorry missed to keep -hackers in previous mail.
From: Amit Kapila amit.kapil...@gmail.com
Date: Mon, Oct 7, 2013 at 11:37 AM
Subject: Re: Patch for reserved connections for replication users
To: Robert Haas robertmh...@gmail.com, gibh...@zero-knowledge.org
Cc: Andres Freund and...@2ndquadrant.com, ma...@joh.to
Robert Haas wrote:
On Mon, Aug 5, 2013 at 2:04 AM, Andres Freund
andres(at)2ndquadrant(dot)com wrote:
Hmm. It seems like this match is making MaxConnections no longer mean
the maximum number of connections, but rather the maximum number of
non-replication connections. I don't think I support that
definitional change, and I'm kinda surprised if this is sufficient to
implement it anyway (e.g. see InitProcGlobal()).
I don't think the implementation is correct, but why don't you like the
definitional change? The set of things you can do from replication
connections are completely different from a normal connection. So using
separate pools for them seems to make sense.
That they end up allocating similar internal data seems to be an
implementation detail to me.
Because replication connections are still connections. If I tell
the system I want to allow 100 connections to the server, it should
allow 100 connections, not 110 or 95 or any other number.
I think that to reserve connections for replication, mechanism similar
to superuser_reserved_connections be used rather than auto vacuum
workers or background workers.
This won't change the definition of MaxConnections. Another thing is
that rather than introducing new parameter for replication reserved
connections, it is better to use max_wal_senders as it can serve the
purpose.
Review for replication_reserved_connections-v2.patch, considering we
are going to use mechanism similar to superuser_reserved_connections
and won't allow definition of MaxConnections to change.
1. /* the extra unit accounts for the autovacuum launcher */
MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
- + max_worker_processes;
+ + max_worker_processes + max_wal_senders;
Above changes are not required.
2.
+ if ((!am_superuser !am_walsender)
ReservedBackends 0
!HaveNFreeProcs(ReservedBackends))
Change the check as you have in patch-1 for ReserveReplicationConnections
if (!am_superuser
(max_wal_senders 0 || ReservedBackends 0)
!HaveNFreeProcs(max_wal_senders + ReservedBackends))
ereport(FATAL,
(errcode(ERRCODE_TOO_MANY_CONNECTIONS),
errmsg(remaining connection slots are reserved for replication and
superuser connections)));
3. In guc.c, change description of max_wal_senders similar to
superuser_reserved_connections at below place:
{max_wal_senders, PGC_POSTMASTER, REPLICATION_SENDING,
gettext_noop(Sets the maximum number of simultaneously running WAL
sender processes.),
4. With this approach, there is no need to change InitProcGlobal(), as
it is used to keep track bgworkerFreeProcs and autovacFreeProcs, for
others it use freeProcs.
5. Below description in config.sgml needs to be changed for
superuser_reserved_connections to include the effect of max_wal_enders
in reserved connections.
Whenever the number of active concurrent connections is at least
max_connections minus superuser_reserved_connections, new connections
will be accepted only for superusers, and no new replication
connections will be accepted.
6. Also similar description should be added to max_wal_senders
configuration parameter.
7. Below comment needs to be updated, as it assumes only superuser
reserved connections as part of MaxConnections limit.
/*
* ReservedBackends is the number of backends reserved for superuser use.
* This number is taken out of the pool size given by MaxBackends so
* number of backend slots available to non-superusers is
* (MaxBackends - ReservedBackends). Note what this really means is
* if there are = ReservedBackends connections available, only superusers
* can make new connections --- pre-existing superuser connections don't
* count against the limit.
*/
int ReservedBackends;
8. Also we can add comment on top of definition for max_wal_senders
similar to ReservedBackends.
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] SSI freezing bug
On 06.10.2013 20:34, Kevin Grittner wrote: Note this comment, which I think was written by Heikki back when there was a lot more benchmarking and profiling of SSI going on: * Because a particular target might become obsolete, due to update to a new * version, before the reading transaction is obsolete, we need some way to * prevent errors from reuse of a tuple ID. Rather than attempting to clean * up the targets as the related tuples are pruned or vacuumed, we check the * xmin on access.This should be far less costly. Based on what this patch looks like, I'm afraid he may have been right when he wrote that. In any event, after the exercise of developing a first draft of searching for predicate locks to clean up every time a tuple is pruned or vacuumed, I continue to feel strongly that the previously-posted patch, which only takes action when tuples are frozen by a vacuum process, is much more suitable for backpatching. I don't think we should switch to anything resembling the attached without some careful benchmarking. Hmm, you're probably right. I was thinking that the overhead of scanning the lock hash on a regular vacuum is negligible, but I didn't consider pruning. It might be significant there. I'd like to give this line of investigation some more thought: On 04.10.2013 07:14, Dan Ports wrote: On Thu, Oct 03, 2013 at 06:19:49AM -0700, Kevin Grittner wrote: Heikki Linnakangashlinnakan...@vmware.com wrote: IMHO it would be better to remove xmin from the lock key, and vacuum away the old predicate locks when the corresponding tuple is vacuumed. The xmin field is only required to handle the case that a tuple is vacuumed, and a new unrelated tuple is inserted to the same slot. Removing the lock when the tuple is removed fixes that. This seems definitely safe: we need the predicate locks to determine if someone is modifying a tuple we read, and certainly if it's eligible for vacuum nobody's going to be modifying that tuple anymore. In fact, I cannot even come up with a situation where you would have a problem if we just removed xmin from the key, even if we didn't vacuum away old locks. I don't think the old lock can conflict with anything that would see the new tuple that gets inserted in the same slot. I have a feeling that you could probably prove that if you stare long enough at the diagram of a dangerous structure and the properties required for a conflict. This would also be safe, in the sense that it's OK to flag a conflict even if one doesn't exist. I'm not convinced that it isn't possible to have false positives this way. I think it's possible for a tuple to be vacuumed away and the ctid reused before the predicate locks on it are eligible for cleanup. (In fact, isn't this what was happening in the thread Kevin linked?) Time to stare at the dangerous structure again: SSI is based on the observation [2] that each snapshot isolation anomaly corresponds to a cycle that contains a dangerous structure of two adjacent rw-conflict edges: Tin -- Tpivot -- Tout rw rw Furthermore, Tout must commit first. The following are true: * A transaction can only hold a predicate lock on a tuple that's visible to it. * A tuple that's visible to Tin or Tpivot cannot be vacuumed away until Tout commits. When updating a tuple, CheckTargetForConflictsIn() only marks a conflict if the transaction holding the predicate lock overlapped with the updating transaction. And if a tuple is vacuumed away and the slot is reused, an transaction updating the new tuple cannot overlap with any transaction holding a lock on the old tuple. Otherwise the tuple wouldn't have been eligible for vacuuming in the first place. So I don't think you can ever get a false conflict because of slot reuse. And if there's a hole in that thinking I can't see right now, the worst that will happen is some unnecessary conflicts, ie. it's still correct. It surely can't be worse than upgrading the lock to a page-level lock, which would also create unnecessary conflicts. Summary: IMHO we should just remove xmin from the predicate lock tag. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] old warning in docs
Given that we have not supported releases older than 8.3 for quite a while, do we need to keep this in extend.sgml any longer? caution para Changing varnamePG_CONFIG/varname only works when building against productnamePostgreSQL/productname 8.3 or later. With older releases it does not work to set it to anything except literalpg_config/; you must alter your varnamePATH/ to select the installation to build against. /para /caution cheers andrew -- 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] pgbench progress report improvements - split 3 v2 - A
pgbench already offers two schedules of pgbench --initialize messaging, message-per-100k-rows and message-per-5s. A user too picky to find satisfaction in either option can filter the messages through grep, sed et al. We patched pgbench on two occasions during the 9.3 cycle to arrive at that status quo. Had I participated, I may well have voted for something like your proposal over pgbench --quiet. Now that we've released --quiet, a proposal for an additional message schedule option needs to depict a clear and convincing step forward. This proposal does not rise to that level. The step forward is to have the same options apply to *both* modes, instead of options for one mode (default 100k or --quiet 5s), and a convenient --progress (adjustable seconds) for the other. It is convincing to me because I hate commands with non orthogonal options, as I'm sure not to notice that one option is for one mode and the other for another, and to get caught by it. But this is clearly a matter of design taste. -- 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] Re: custom hash-based COUNT(DISTINCT) aggregate - unexpectedly high memory consumption
On Mon, Oct 07, 2013 at 12:41:58AM +0200, Tomas Vondra wrote: 2. Consider using a simpler/faster hash function, like FNV[1] or Jenkins[2]. For fun, try not hashing those ints at all and see how that performs (that, I think, is what you get from HashSetint in Java/C#). I've used crc32 mostly because it's easily available in the code (i.e. I'm lazy), but I've done some quick research / primitive benchmarking too. For example hashing 2e9 integers takes this much time: FNV-1 = 11.9 FNV-1a = 11.9 jenkins = 38.8 crc32 = 32.0 So it's not really slow and the uniformity seems to be rather good. I'll try FNV in the future, however I don't think that's the main issue right now. Hi Tomas, If you are going to use a function that is not currently in the code, please consider xxhash: http://code.google.com/p/xxhash/ Here are some benchmarks for some of the faster hash functions: NameSpeed Q.Score Author xxHash 5.4 GB/s 10 MumurHash 3a2.7 GB/s 10 Austin Appleby SpookyHash 2.0 GB/s 10 Bob Jenkins SBox1.4 GB/s 9 Bret Mulvey Lookup3 1.2 GB/s 9 Bob Jenkins CityHash64 1.05 GB/s10 Pike Alakuijala FNV 0.55 GB/s 5 Fowler, Noll, Vo CRC32 0.43 GB/s 9 MD5-32 0.33 GB/s10 Ronald L. Rivest SHA1-32 0.28 GB/s10 Regards, Ken -- 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] SSI freezing bug
Heikki Linnakangas hlinnakan...@vmware.com wrote:
So I don't think you can ever get a false conflict because of
slot reuse.
I spent some time looking at this, and I now agree.
And if there's a hole in that thinking I can't see right now, the
worst that will happen is some unnecessary conflicts, ie. it's
still correct.
That is definitely true; no doubt about that part.
Summary: IMHO we should just remove xmin from the predicate lock
tag.
I spent some time trying to see how the vacuum could happen at a
point that could cause a false positive, and was unable to do so.
Even if that is just a failure of imagination on my part, the above
argument that it doesn't produce incorrect results still holds. I
think the fact that I couldn't find a sequence of events which
would trigger a false positive suggests it would be a rare case,
anyway.
I found the original bug report which led to the addition of xmin
to the tag, and it was because we were still carrying tuple locks
forward to new versions of those locks at the time. This was later
proven to be unnecessary, which simplified other code; we
apparently missed a trick in not removing xmin from the lock tag at
that point. Since leaving it there has now been shown to *cause* a
bug, I'm inclined to agree that we should simply remove it, and
backpatch that.
Patch attached. Any objections to applying that Real Soon Now?
(When, exactly is the deadline to make today's minor release
cut-off?)
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company*** a/src/backend/storage/lmgr/predicate.c
--- b/src/backend/storage/lmgr/predicate.c
***
*** 156,164
* PredicateLockTuple(Relation relation, HeapTuple tuple,
* Snapshot snapshot)
* PredicateLockPageSplit(Relation relation, BlockNumber oldblkno,
! * BlockNumber newblkno);
* PredicateLockPageCombine(Relation relation, BlockNumber oldblkno,
! * BlockNumber newblkno);
* TransferPredicateLocksToHeapRelation(Relation relation)
* ReleasePredicateLocks(bool isCommit)
*
--- 156,164
* PredicateLockTuple(Relation relation, HeapTuple tuple,
* Snapshot snapshot)
* PredicateLockPageSplit(Relation relation, BlockNumber oldblkno,
! * BlockNumber newblkno)
* PredicateLockPageCombine(Relation relation, BlockNumber oldblkno,
! * BlockNumber newblkno)
* TransferPredicateLocksToHeapRelation(Relation relation)
* ReleasePredicateLocks(bool isCommit)
*
***
*** 381,387 static SHM_QUEUE *FinishedSerializableTransactions;
* this entry, you can ensure that there's enough scratch space available for
* inserting one entry in the hash table. This is an otherwise-invalid tag.
*/
! static const PREDICATELOCKTARGETTAG ScratchTargetTag = {0, 0, 0, 0, 0};
static uint32 ScratchTargetTagHash;
static int ScratchPartitionLock;
--- 381,387
* this entry, you can ensure that there's enough scratch space available for
* inserting one entry in the hash table. This is an otherwise-invalid tag.
*/
! static const PREDICATELOCKTARGETTAG ScratchTargetTag = {0, 0, 0, 0};
static uint32 ScratchTargetTagHash;
static int ScratchPartitionLock;
***
*** 2492,2499 PredicateLockTuple(Relation relation, HeapTuple tuple, Snapshot snapshot)
}
}
}
- else
- targetxmin = InvalidTransactionId;
/*
* Do quick-but-not-definitive test for a relation lock first. This will
--- 2492,2497
***
*** 2512,2519 PredicateLockTuple(Relation relation, HeapTuple tuple, Snapshot snapshot)
relation-rd_node.dbNode,
relation-rd_id,
ItemPointerGetBlockNumber(tid),
! ItemPointerGetOffsetNumber(tid),
! targetxmin);
PredicateLockAcquire(tag);
}
--- 2510,2516
relation-rd_node.dbNode,
relation-rd_id,
ItemPointerGetBlockNumber(tid),
! ItemPointerGetOffsetNumber(tid));
PredicateLockAcquire(tag);
}
***
*** 4283,4290 CheckForSerializableConflictIn(Relation relation, HeapTuple tuple,
relation-rd_node.dbNode,
relation-rd_id,
ItemPointerGetBlockNumber((tuple-t_data-t_ctid)),
! ItemPointerGetOffsetNumber((tuple-t_data-t_ctid)),
! HeapTupleHeaderGetXmin(tuple-t_data));
CheckTargetForConflictsIn(targettag);
}
--- 4280,4286
relation-rd_node.dbNode,
relation-rd_id,
ItemPointerGetBlockNumber((tuple-t_data-t_ctid)),
! ItemPointerGetOffsetNumber((tuple-t_data-t_ctid)));
CheckTargetForConflictsIn(targettag);
}
*** a/src/include/storage/predicate.h
--- b/src/include/storage/predicate.h
***
*** 52,57 extern void PredicateLockTuple(Relation relation, HeapTuple tuple, Snapshot snap
--- 52,58
extern void PredicateLockPageSplit(Relation relation, BlockNumber oldblkno, BlockNumber
Re: [HACKERS] SSI freezing bug
Kevin Grittner kgri...@ymail.com wrote: Patch attached. Any objections to applying that Real Soon Now? Oh, without the new line in predicate.h. -- Kevin Grittner EDB: 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: [HACKERS] logical changeset generation v6.2
On 10/03/2013 04:00 PM, Andres Freund wrote: Ok, there were a couple of bugs because I thought mxacts wouldn't need to be supported. So far your testcase doesn't crash the database anymore - it spews some internal errors though, so I am not sure if it's entirely fixed for you. Thanks for testing and helping! I've pushed the changes to the git tree, they aren't squashed yet and there's some further outstanding stuff, so I won't repost the series yet. Greetings, Andres Freund When I run your updated version (from friday, not what you posted today) against a more recent version of my slony changes I can get the test case to pass 2/3 'rd of the time. The failures are due to an issue in slon itself that I need to fix. I see lots of 0LOG: tx with subtxn 58836 but they seem harmless. Thanks -- 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] SSI freezing bug
On 2013-10-07 06:44:19 -0700, Kevin Grittner wrote: Patch attached. Any objections to applying that Real Soon Now? (When, exactly is the deadline to make today's minor release cut-off?) Maybe it's overly careful, but I personally slightly vote for applying it after the backbranch releases. The current behaviour doesn't have any harsh consequences and mostly reproduceable in artifical scenarios and the logic here is complex enough that we might miss something. A day just doesn't leave much time to noticing any issues. 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: [HACKERS] logical changeset generation v6.2
On 2013-10-07 09:56:11 -0400, Steve Singer wrote: On 10/03/2013 04:00 PM, Andres Freund wrote: Ok, there were a couple of bugs because I thought mxacts wouldn't need to be supported. So far your testcase doesn't crash the database anymore - it spews some internal errors though, so I am not sure if it's entirely fixed for you. Thanks for testing and helping! I've pushed the changes to the git tree, they aren't squashed yet and there's some further outstanding stuff, so I won't repost the series yet. Greetings, Andres Freund When I run your updated version (from friday, not what you posted today) against a more recent version of my slony changes I can get the test case to pass 2/3 'rd of the time. The failures are due to an issue in slon itself that I need to fix. Cool. I see lots of 0LOG: tx with subtxn 58836 Yes, those are completely harmless. And should, in fact, be removed. I guess I should add the todo entry: * make a pass over all elog/ereport an make sure they have the correct log level et al. 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: [HACKERS] ToDo: fast update of arrays with fixed length fields for PL/pgSQL
Hello I fixed patch - there was a missing cast to domain when it was used (and all regress tests are ok now). a some performance tests set array_fast_update to off; postgres=# select fill_2d_array(300,300); fill_2d_array ─── 9 (1 row) Time: 33570.087 ms postgres=# set array_fast_update to on; SET Time: 0.279 ms postgres=# select fill_2d_array(300,300); fill_2d_array ─── 9 (1 row) Time: 505.202 ms CREATE OR REPLACE FUNCTION public.quicksort(l integer, r integer, a integer[]) RETURNS integer[] LANGUAGE plpgsql IMMUTABLE STRICT AS $function$ DECLARE akt int[] = a; i integer := l; j integer := r; x integer = akt[(l+r) / 2]; w integer; BEGIN LOOP WHILE akt[i] x LOOP i := i + 1; END LOOP; WHILE x akt[j] loop j := j - 1; END LOOP; IF i = j THEN w := akt[i]; akt[i] := akt[j]; akt[j] := w; i := i + 1; j := j - 1; END IF; EXIT WHEN i j; END LOOP; IF l j THEN akt := quicksort(l,j,akt); END IF; IF i r then akt := quicksort(i,r,akt); END IF; RETURN akt; END; $function$ postgres=# set array_fast_update to off; SET Time: 0.282 ms postgres=# SELECT array_upper(quicksort(1,21000,array_agg(a)),1) FROM test; array_upper ─ 21000 (1 row) Time: 5086.781 ms postgres=# set array_fast_update to on; SET Time: 0.702 ms postgres=# SELECT array_upper(quicksort(1,21000,array_agg(a)),1) FROM test; array_upper ─ 21000 (1 row) Time: 1751.920 ms So in first test - fast update is 66x faster, second test - 3x faster I found so for small arrays (about 1000 fields) a difference is not significant. This code can be cleaned (a domains can be better optimized generally - a IO cast can be expensive for large arrays and domain_check can be used there instead), but it is good enough for discussion if we would this optimization or not. Regards Pavel 2013/10/3 Pavel Stehule pavel.steh...@gmail.com Hello a very ugly test shows a possibility about 100% speedup on reported example (on small arrays, a patch is buggy and doesn't work for larger arrays). I updated a code to be read only CREATE OR REPLACE FUNCTION public.fill_2d_array(rows integer, cols integer) RETURNS integer LANGUAGE plpgsql AS $function$ DECLARE img double precision[][]; i integer; j integer; cont integer; r double precision; BEGIN img := ARRAY( SELECT 0 FROM generate_series(1, rows * cols) ) ; cont:= 0; For i IN 1..rows LOOP For j IN 1..cols LOOP r := img[i * cols + j]; r := (i * cols + j)::double precision; cont := cont + 1; --raise notice '%', img; END LOOP; END LOOP; return cont; END; $function$ It exec all expressions -- original postgres=# select fill_2d_array(200,200); fill_2d_array --- 4 (1 row) Time: 12726.117 ms -- read only version postgres=# select fill_2d_array(200,200); fill_2d_array --- 4 (1 row) Time: 245.894 ms so there is about 50x slowdown 2013/10/3 Pavel Stehule pavel.steh...@gmail.com 2013/10/3 Tom Lane t...@sss.pgh.pa.us Pavel Stehule pavel.steh...@gmail.com writes: If you can do a update of some array in plpgsql now, then you have to work with local copy only. It is a necessary precondition, and I am think it is valid. If the proposal only relates to assignments to elements of plpgsql local variables, it's probably safe, but it's also probably not of much value. plpgsql has enough overhead that I'm doubting you'd get much real-world speedup. I'm also not very excited about putting even more low-level knowledge about array representation into plpgsql. I looked to code, and I am thinking so this can be done inside array related routines. We just have to signalize request for inplace update (if we have a local copy). I have not idea, how significant speedup can be (if any), but current behave is not friendly (and for multidimensional arrays there are no workaround), so it is interesting way - and long time I though about some similar optimization. Regards Pavel regards, tom lane fast-array-update.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] ToDo: fast update of arrays with fixed length fields for PL/pgSQL
On 2013-10-07 16:00:54 +0200, Pavel Stehule wrote: /* * We need to do subscript evaluation, which might require @@ -4321,6 +4322,14 @@ exec_assign_value(PLpgSQL_execstate *estate, oldarrayval = (ArrayType *) DatumGetPointer(oldarraydatum); /* + * support fast update for array scalar variable is enabled only + * when target is a scalar variable and variable holds a local + * copy of some array. + */ + inplace_update = (((PLpgSQL_datum *) target)-dtype == PLPGSQL_DTYPE_VAR + ((PLpgSQL_var *) target)-freeval); + + /* * Build the modified array value. */ Will this recognize if the local Datum is just a reference to an external toast Datum (i.e. varattrib_1b_e)? I don't know much about plpgsql's implementation, so please excuse if the question is stupid. 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: [HACKERS] SSI freezing bug
On 07.10.2013 16:58, Andres Freund wrote: On 2013-10-07 06:44:19 -0700, Kevin Grittner wrote: Patch attached. Any objections to applying that Real Soon Now? (When, exactly is the deadline to make today's minor release cut-off?) Maybe it's overly careful, but I personally slightly vote for applying it after the backbranch releases. The current behaviour doesn't have any harsh consequences and mostly reproduceable in artifical scenarios and the logic here is complex enough that we might miss something. Well, it's fairly harsh if the feature isn't working as advertised. A day just doesn't leave much time to noticing any issues. It's less than ideal, I agree, but it doesn't seem better to me to just leave the bug unfixed for another minor release. Even if we sit on this for another few months, I don't think we'll get any meaningful amount of extra testing on it. - Heikki -- 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] SSI freezing bug
On 07.10.2013 16:44, Kevin Grittner wrote: Heikki Linnakangashlinnakan...@vmware.com wrote: So I don't think you can ever get a false conflict because of slot reuse. I spent some time looking at this, and I now agree. And if there's a hole in that thinking I can't see right now, the worst that will happen is some unnecessary conflicts, ie. it's still correct. That is definitely true; no doubt about that part. Summary: IMHO we should just remove xmin from the predicate lock tag. I spent some time trying to see how the vacuum could happen at a point that could cause a false positive, and was unable to do so. Even if that is just a failure of imagination on my part, the above argument that it doesn't produce incorrect results still holds. I think the fact that I couldn't find a sequence of events which would trigger a false positive suggests it would be a rare case, anyway. I found the original bug report which led to the addition of xmin to the tag, and it was because we were still carrying tuple locks forward to new versions of those locks at the time. This was later proven to be unnecessary, which simplified other code; we apparently missed a trick in not removing xmin from the lock tag at that point. Since leaving it there has now been shown to *cause* a bug, I'm inclined to agree that we should simply remove it, and backpatch that. Patch attached. Any objections to applying that Real Soon Now? (When, exactly is the deadline to make today's minor release cut-off?) A comment somewhere would be nice to explain why we're no longer worried about confusing an old tuple version with a new tuple in the same slot. Not sure where. - Heikki -- 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] SSI freezing bug
On 2013-10-07 17:07:16 +0300, Heikki Linnakangas wrote: On 07.10.2013 16:58, Andres Freund wrote: On 2013-10-07 06:44:19 -0700, Kevin Grittner wrote: Patch attached. Any objections to applying that Real Soon Now? (When, exactly is the deadline to make today's minor release cut-off?) Maybe it's overly careful, but I personally slightly vote for applying it after the backbranch releases. The current behaviour doesn't have any harsh consequences and mostly reproduceable in artifical scenarios and the logic here is complex enough that we might miss something. Well, it's fairly harsh if the feature isn't working as advertised. Well, you need to have a predicate lock on a tuple that's getting frozen (i.e. hasn't been written to for 50mio+ xids) and you need to have an update during the time that predicate lock is held. That's not too likely without explicit VACUUM FREEZEs interleaved there somewhere. A day just doesn't leave much time to noticing any issues. It's less than ideal, I agree, but it doesn't seem better to me to just leave the bug unfixed for another minor release. Even if we sit on this for another few months, I don't think we'll get any meaningful amount of extra testing on it. Yea, maybe. Although HEAD does get some testing... 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: [HACKERS] SSI freezing bug
Heikki Linnakangas hlinnakan...@vmware.com wrote: Well, it's fairly harsh if the feature isn't working as advertised. Right -- there are people counting on serializable transaction to avoid data corruption (creation of data which violates the business rules which they believe are being enforced). A tuple freeze at the wrong moment could currently break that. The patch allows the guarantee to hold. The fact that this bug is only seen if there is a very-long-running transaction or if VACUUM FREEZE is run while data-modifying transactions are active does not eliminate the fact that without this patch data can be corrupted. -- Kevin Grittner EDB: 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: [HACKERS] SSI freezing bug
Andres Freund and...@2ndquadrant.com wrote: On 2013-10-07 06:44:19 -0700, Kevin Grittner wrote: Patch attached. Any objections to applying that Real Soon Now? (When, exactly is the deadline to make today's minor release cut-off?) Maybe it's overly careful, but I personally slightly vote for applying it after the backbranch releases. The current behaviour doesn't have any harsh consequences and mostly reproduceable in artifical scenarios and the logic here is complex enough that we might miss something. A day just doesn't leave much time to noticing any issues. I grant that the bug in existing production code is not likely to get hit very often, but it is a bug; the new isolation test shows the bug clearly and shows that the suggested patch fixes it. What tips the scales for me is that the only possible downside if we missed something is an occasional false positive serialization failure, which does not break correctness -- we try to minimize those for performance reasons, but the algorithm allows them and they currently do happen. -- Kevin Grittner EDB: 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: [HACKERS] ToDo: fast update of arrays with fixed length fields for PL/pgSQL
2013/10/7 Andres Freund and...@2ndquadrant.com On 2013-10-07 16:00:54 +0200, Pavel Stehule wrote: /* * We need to do subscript evaluation, which might require @@ -4321,6 +4322,14 @@ exec_assign_value(PLpgSQL_execstate *estate, oldarrayval = (ArrayType *) DatumGetPointer(oldarraydatum); /* + * support fast update for array scalar variable is enabled only + * when target is a scalar variable and variable holds a local + * copy of some array. + */ + inplace_update = (((PLpgSQL_datum *) target)-dtype == PLPGSQL_DTYPE_VAR + ((PLpgSQL_var *) target)-freeval); + + /* * Build the modified array value. */ Will this recognize if the local Datum is just a reference to an external toast Datum (i.e. varattrib_1b_e)? this works on plpgsql local copies only (when cleaning is controlled by plpgsql) - so it should be untoasted every time. btw when I tested this patch I found a second plpgsql array issue - repeated untoasting :( and we should not forget it. I don't know much about plpgsql's implementation, so please excuse if the question is stupid. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] SSI freezing bug
Kevin Grittner kgri...@ymail.com wrote: Andres Freund and...@2ndquadrant.com wrote: On 2013-10-07 06:44:19 -0700, Kevin Grittner wrote: Patch attached. Any objections to applying that Real Soon Now? (When, exactly is the deadline to make today's minor release cut-off?) Maybe it's overly careful, but I personally slightly vote for applying it after the backbranch releases. The current behaviour doesn't have any harsh consequences and mostly reproduceable in artifical scenarios and the logic here is complex enough that we might miss something. A day just doesn't leave much time to noticing any issues. I grant that the bug in existing production code is not likely to get hit very often, but it is a bug; the new isolation test shows the bug clearly and shows that the suggested patch fixes it. What tips the scales for me is that the only possible downside if we missed something is an occasional false positive serialization failure, which does not break correctness -- we try to minimize those for performance reasons, but the algorithm allows them and they currently do happen. I am, of course, continuing to review this. There might be a problem if someone applies this fix while any prepared transactions are pending. Still investigating the impact and possible fixes. -- Kevin Grittner EDB: 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: [HACKERS] Re: custom hash-based COUNT(DISTINCT) aggregate - unexpectedly high memory consumption
Sent from my iPad On 07-Oct-2013, at 4:11, Tomas Vondra t...@fuzzy.cz wrote: On 6.10.2013 20:37, Tomáš Janoušek wrote: Hi, On Sat, Oct 05, 2013 at 08:22:54PM +0200, Tomas Vondra wrote: I'm on 64-bit architecture and the example works with int32, which means the sizes should be about this: hash_element_t = 20B hash_bucket_t = 4B + (20B * items in the bucket [in steps of 5]) hash_table_t = 4B + space for buckets In the example above, there's ~20k unique values in each group. The threshold is 20 items per bucket on average, so that's 1024 buckets, and the buckets are almost full. So for single group, the hash table size is about 4B + 1024 * (4B + 20 * 20B) = 413700B = ~ 400 kB There are 4000 groups, so the total estimate is ~1.6GB in total. However when executed (9.2, 9.3 and HEAD behave exactly the same), the query consumes almost ~5GB of RAM (excluding shared buffers). I think the missing thing is the memory allocator bookkeeping overhead. You're assuming that hash_element_t.value takes 8B for the pointer and 4B for the value itself, but using malloc it takes another at least 20 bytes, and from a quick glance at backend/utils/mmgr/aset.c it seems that palloc is certainly not without its overhead either. Also, each additional level of pointers adds execution overhead and increases the likelihood of cache misses. I'd suggest a few improvements, if I may: 1. Drop hash_element_t altogether, store length in hash_bucket_t and alloc hash_bucket_t.items of size nitems * length bytes. I doubt that storing the hash values has a benefit worth the storage and code complexity overhead (you're storing fixed-size ints, not large blobs that are expensive to compare and hash). Good idea - I'll move the length to the hash table. You're right that keeping the hash for int32/64 values is probably useless, however I planned to eventually extend the code to support larger values (possibly even varlena types, i.e. text/bytea). So I'll keep this for now, but I'll keep this as a possible optimization. 2. Consider using a simpler/faster hash function, like FNV[1] or Jenkins[2]. For fun, try not hashing those ints at all and see how that performs (that, I think, is what you get from HashSetint in Java/C#). I've used crc32 mostly because it's easily available in the code (i.e. I'm lazy), but I've done some quick research / primitive benchmarking too. For example hashing 2e9 integers takes this much time: FNV-1 = 11.9 FNV-1a = 11.9 jenkins = 38.8 crc32 = 32.0 So it's not really slow and the uniformity seems to be rather good. I'll try FNV in the future, however I don't think that's the main issue right now. 3. Consider dropping buckets in favor of open addressing (linear probing, quadratic, whatever). This avoids another level of pointer indirection. OK, this sounds really interesting. It should be fairly straightforward for fixed-length data types (in that case I can get rid of the pointers altogether). Consider the aspects associated with open addressing.Open addressing can quickly lead to growth in the main table.Also, chaining is a much cleaner way of collision resolution,IMHO. It's been a few years since I've done stuff this low level, so I won't go into suggesting a different data structure -- I have honestly no idea what's the best way to count the number of distinct integers in a list. Thanks for the hints! Tomas -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- 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] old warning in docs
On Mon, Oct 07, 2013 at 07:51:44AM -0400, Andrew Dunstan wrote: Given that we have not supported releases older than 8.3 for quite a while, do we need to keep this in extend.sgml any longer? caution para Changing varnamePG_CONFIG/varname only works when building against productnamePostgreSQL/productname 8.3 or later. With older releases it does not work to set it to anything except literalpg_config/; you must alter your varnamePATH/ to select the installation to build against. /para /caution I say bin it. That reminds me. There are probably a lot of places in the docs that refer to versions of PostgreSQL a good bit older than 8.3. Will grep and patch as I get the time. Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] segfault with contrib lo
I was using the lo contrib a few days ago and wasn't paying attention, and forgot the for each row in the create trigger command... PostgreSQL segfaulted, when the trigger tried to access the row's attributes. Please find attached a patch to control that the trigger is correctly defined (as in the example): a before trigger, for each row, and a parameter (if the parameter was omitted, it segfaulted too). I hope I did this correctly. Regards, Marc.diff --git a/contrib/lo/lo.c b/contrib/lo/lo.c index 9dbbbce..2b9477a 100644 --- a/contrib/lo/lo.c +++ b/contrib/lo/lo.c @@ -50,6 +50,13 @@ lo_manage(PG_FUNCTION_ARGS) tupdesc = trigdata-tg_relation-rd_att; args = trigdata-tg_trigger-tgargs; + if (!TRIGGER_FIRED_FOR_ROW(trigdata-tg_event)) /* internal error */ + elog(ERROR, not fired by a for each row trigger); + + if (!TRIGGER_FIRED_BEFORE(trigdata-tg_event)) /* internal error */ + elog(ERROR, not fired as a before trigger); + + /* tuple to return to Executor */ if (TRIGGER_FIRED_BY_UPDATE(trigdata-tg_event)) rettuple = newtuple; @@ -59,6 +66,9 @@ lo_manage(PG_FUNCTION_ARGS) /* Are we deleting the row? */ isdelete = TRIGGER_FIRED_BY_DELETE(trigdata-tg_event); + if (args == NULL) /* internal error */ + elog (ERROR, no column name provided in the trigger definition); + /* Get the column we're interested in */ attnum = SPI_fnumber(tupdesc, args[0]); -- 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] space reserved for WAL record does not match what was written: panic on windows
On Fri, Oct 4, 2013 at 8:19 AM, Andres Freund and...@2ndquadrant.com wrote: Could it be that MAXALIGN/TYPEALIGN doesn't really work for values bigger than 32bit? #define MAXALIGN(LEN) TYPEALIGN(MAXIMUM_ALIGNOF, (LEN)) #define TYPEALIGN(ALIGNVAL,LEN) \ (((intptr_t) (LEN) + ((ALIGNVAL) - 1)) ~((intptr_t) ((ALIGNVAL) - 1))) Isn't the problem, more specifically, that it doesn't work for values larger than an intptr_t? And does that indicate that intptr_t is the wrong type to be using here? -- 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: [HACKERS] mvcc catalo gsnapshots and TopTransactionContext
On Fri, Oct 4, 2013 at 3:20 PM, Andres Freund and...@2ndquadrant.com wrote: On 2013-10-04 15:15:36 -0400, Robert Haas wrote: Andres, are you (or is anyone) going to try to fix this assertion failure? I think short term replacing it by IsTransactionOrTransactionBlock() is the way to go. Entirely restructuring how cache invalidation in the abort path works is not a small task. Well, if we're going to go that route, how about something like the attached? I included the assert-change per se, an explanatory comment, and the test case that Noah devised to cause the current assertion to fail. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company weaken-searchcatcache-assert.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] Patch: FORCE_NULL option for copy COPY in CSV mode
On Sat, Oct 5, 2013 at 7:38 AM, Amit Kapila amit.kapil...@gmail.com wrote: On Sun, Sep 29, 2013 at 1:39 PM, Ian Lawrence Barwick barw...@gmail.com wrote: Hi, This patch implements the following TODO item: Allow COPY in CSV mode to control whether a quoted zero-length string is treated as NULL Currently this is always treated as a zero-length string, which generates an error when loading into an integer column Re: [PATCHES] allow CSV quote in NULL http://archives.postgresql.org/pgsql-hackers/2007-07/msg00905.php http://wiki.postgresql.org/wiki/Todo#COPY I had a very definite use-case for this functionality recently while importing CSV files generated by Oracle, and was somewhat frustrated by the existence of a FORCE_NOT_NULL option for specific columns, but not one for FORCE_NULL. While going through documentation of this patch to understand it's usage, I found a small mistake. + Force the specified columns' values to be converted to literalNULL/ + if the value contains an empty string. It seems quote after columns is wrong. That's a correct plural possessive in English, but in might be better worded as Force any empty string encountered in the input for the specified columns to be interpreted as a NULL value. Also if your use case is to treat empty strings as NULL (as per above documentation), can't it be handled with WITH NULL AS option. For example, something like: postgres=# COPY testnull FROM stdin with CSV NULL AS E''; Enter data to be copied followed by a newline. End with a backslash and a period on a line by itself. 50, \. postgres=# select * from testnull; a | b +-- 50 | NULL (1 row) Good point. If this patch is just implementing something that can already be done with another syntax, we don't need it. -- 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: [HACKERS] Patch: FORCE_NULL option for copy COPY in CSV mode
On 10/07/2013 03:06 PM, Robert Haas wrote: Also if your use case is to treat empty strings as NULL (as per above documentation), can't it be handled with WITH NULL AS option. For example, something like: postgres=# COPY testnull FROM stdin with CSV NULL AS E''; Enter data to be copied followed by a newline. End with a backslash and a period on a line by itself. 50, \. postgres=# select * from testnull; a | b +-- 50 | NULL (1 row) Good point. If this patch is just implementing something that can already be done with another syntax, we don't need it. Isn't the point of this option to allow a *quoted* empty string to be forced to NULL? If so, this is not testing the same case - in fact the COPY command above just makes explicit the default CSV NULL setting anyway. cheers andrew -- 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] plpgsql.print_strict_params
On Sat, Oct 5, 2013 at 6:15 PM, Marko Tiikkaja ma...@joh.to wrote: Something like the attached? Looks good to me. Committed. -- 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: [HACKERS] Re: custom hash-based COUNT(DISTINCT) aggregate - unexpectedly high memory consumption
On 7.10.2013 14:50, k...@rice.edu wrote: On Mon, Oct 07, 2013 at 12:41:58AM +0200, Tomas Vondra wrote: 2. Consider using a simpler/faster hash function, like FNV[1] or Jenkins[2]. For fun, try not hashing those ints at all and see how that performs (that, I think, is what you get from HashSetint in Java/C#). I've used crc32 mostly because it's easily available in the code (i.e. I'm lazy), but I've done some quick research / primitive benchmarking too. For example hashing 2e9 integers takes this much time: FNV-1 = 11.9 FNV-1a = 11.9 jenkins = 38.8 crc32 = 32.0 So it's not really slow and the uniformity seems to be rather good. I'll try FNV in the future, however I don't think that's the main issue right now. Hi Tomas, If you are going to use a function that is not currently in the code, please consider xxhash: http://code.google.com/p/xxhash/ Here are some benchmarks for some of the faster hash functions: NameSpeed Q.Score Author xxHash 5.4 GB/s 10 MumurHash 3a2.7 GB/s 10 Austin Appleby SpookyHash 2.0 GB/s 10 Bob Jenkins SBox1.4 GB/s 9 Bret Mulvey Lookup3 1.2 GB/s 9 Bob Jenkins CityHash64 1.05 GB/s10 Pike Alakuijala FNV 0.55 GB/s 5 Fowler, Noll, Vo CRC32 0.43 GB/s 9 MD5-32 0.33 GB/s10 Ronald L. Rivest SHA1-32 0.28 GB/s10 Hi, thanks for the link. I repeated the simple benchmark (code is here: http://pastebin.com/e9BS03MA) with these results: FNV duration= 9.821 FNVa duration= 9.753 jenkins duration = 37.658 crc32 duration = 7.127 xxhash duration = 68.814 The only difference is that this time I added -O3 (which I forgot last time). That's probably the reason why CRC32 even faster than FNV. Anyway, xxhash seems to be much slower than the other functions, at least in this particular case. I don't think the poor results necessarily contradict the table of results you've posted, because that's on a large block of random data while I'm hashing very short amounts of data (4B / 8B). So my guess is xxhash init takes much more time than the other algorithms. Chances are it'd be faster on large amounts of data, but that's not really useful. Maybe I'll revisit it in the future if I'll decide to add support for varlena types. Until then I'll stick with crc32 which is fast and has much better Quality score than FNV. Tomas -- 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 DISCARD SEQUENCES command.
On Sat, Oct 5, 2013 at 8:10 AM, Michael Paquier
michael.paqu...@gmail.com wrote:
Here is the test case failing:
=# create sequence foo;
CREATE SEQUENCE
=# select nextval('foo');
nextval
-
1
(1 row)
=# discard sequences ;
DISCARD SEQUENCES
=# select currval('foo');
ERROR: 55000: currval of sequence foo is not yet defined in this session
LOCATION: currval_oid, sequence.c:780
=# select lastval();
The connection to the server was lost. Attempting reset: Failed.
Thanks. I have pushed a fix that I hope will be sufficient.
--
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: [HACKERS] SSI freezing bug
Kevin Grittner kgri...@ymail.com wrote: There might be a problem if someone applies this fix while any prepared transactions are pending. Still investigating the impact and possible fixes. I found a one-line fix for this. It tested without problem. Pushed. -- Kevin Grittner EDB: 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: [HACKERS] insert throw error when year field len 4 for timestamptz datatype
On Mon, Oct 7, 2013 at 12:41 AM, Rushabh Lathia rushabh.lat...@gmail.com wrote: Hmm right it has some inconsistency when year length is 6. But the patch is based on assumption that 5-digit number is a year, because YMD and HMS require at least six digits. Now Year with 6-digit number its getting conflict with YMD and HMS, that the reason its ending up with error. So with patch approach that's an expected behaviour for me. I spent good amount of time on thinking how we can improve the behaviour, or how can be change the assumption about the year field, YMD and HMS. At current point of time it seems difficult to me because postgres date module is tightly build with few assumption and changing that may lead to big project. Not sure but personally I feel that patch which was submitted earlier was definitely good improvement. Any other suggestion or thought for improvement ? I'm not entirely convinced that this patch is heading in the right direction. The thing is, it lets you use 5-digit years always and longer years only in some contexts. So I'm not sure this is really good enough for unambiguous date input. If you want that, you should probably be using trusty YYY-MM-DD format. But if you don't need that, then isn't a five-digit year most likely a typo? This might be a case where throwing an error is actually better than trying to make sense of the input. I don't feel super-strongly about this, but I offer it as a question for reflection. -- 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: [HACKERS] pgbench progress report improvements - split 3 v2 - A
On Sat, Oct 5, 2013 at 6:10 PM, Noah Misch n...@leadboat.com wrote: The sum of the squares of the latencies wraps after 2^63/(10^12 * avg_latency * nclients) seconds. That's unlikely to come up with the ordinary pgbench script, but one can reach it in a few hours when benchmarking a command that runs for many seconds. If we care, we can track the figure as a double. I merely added a comment about it. Using a double seems wise to me. -- 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: [HACKERS] SSI freezing bug
On Mon, Oct 07, 2013 at 12:26:37PM +0300, Heikki Linnakangas wrote: When updating a tuple, CheckTargetForConflictsIn() only marks a conflict if the transaction holding the predicate lock overlapped with the updating transaction. Ah, this is the bit I was forgetting. (I really ought to have remembered that, but it's been a while...) I think it's possible, then, to construct a scenario where a slot is reused before predicate locks on the old tuple are eligible for cleanup -- but those locks will never cause a conflict. So I agree: it's correct to just remove the xmin from the key unconditionally. And this is also true: And if there's a hole in that thinking I can't see right now, the worst that will happen is some unnecessary conflicts, ie. it's still correct. It surely can't be worse than upgrading the lock to a page-level lock, which would also create unnecessary conflicts. Dan -- Dan R. K. PortsUW CSEhttp://drkp.net/ -- 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] SSI freezing bug
On 07.10.2013 22:35, Kevin Grittner wrote: Kevin Grittnerkgri...@ymail.com wrote: There might be a problem if someone applies this fix while any prepared transactions are pending. Still investigating the impact and possible fixes. I found a one-line fix for this. It tested without problem. Good catch. To fix the bug that Hannu pointed out, we also need to apply these fixes: http://www.postgresql.org/message-id/52440266.5040...@vmware.com - Heikki -- 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] space reserved for WAL record does not match what was written: panic on windows
On 2013-10-07 13:25:17 -0400, Robert Haas wrote: On Fri, Oct 4, 2013 at 8:19 AM, Andres Freund and...@2ndquadrant.com wrote: Could it be that MAXALIGN/TYPEALIGN doesn't really work for values bigger than 32bit? #define MAXALIGN(LEN) TYPEALIGN(MAXIMUM_ALIGNOF, (LEN)) #define TYPEALIGN(ALIGNVAL,LEN) \ (((intptr_t) (LEN) + ((ALIGNVAL) - 1)) ~((intptr_t) ((ALIGNVAL) - 1))) Isn't the problem, more specifically, that it doesn't work for values larger than an intptr_t? Well, yes. And intptr_t is 32bit wide on a 32bit platform. And does that indicate that intptr_t is the wrong type to be using here? No, I don't think so. intptr_t is defined to be a integer type to which you can cast a pointer, cast it back and still get the old value. On 32bit platforms it usually will be 32bit wide. All that's fine for the classic usages of TYPEALIGN where it's used on pointers or lengths of stuff stored in memory. Those will always fit in 32bit on a 32bit platform. But here we're using it on explicit 64bit types (XLogRecPtr). Now, you could argue that we should make it use 64bit math everywhere - but I think that might incur quite the price on some 32bit platforms. It's used in the tuple decoding stuff, that's quite the hot path in some workloads. So I guess it's either a separate macro, or we rewrite that piece of code to work slightly differently and work directly on the lenght or such. Maybe we should add a StaticAssert ensuring the TYPEALIGN macro only gets passed 32bit types? 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: [HACKERS] SSI freezing bug
On 07.10.2013 23:45, Heikki Linnakangas wrote: On 07.10.2013 22:35, Kevin Grittner wrote: Kevin Grittnerkgri...@ymail.com wrote: There might be a problem if someone applies this fix while any prepared transactions are pending. Still investigating the impact and possible fixes. I found a one-line fix for this. It tested without problem. Good catch. To fix the bug that Hannu pointed out, we also need to apply these fixes: http://www.postgresql.org/message-id/52440266.5040...@vmware.com Per a chat with Bruce, I'm going to apply that patch now to get it into the minor releases. - Heikki -- 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] mvcc catalo gsnapshots and TopTransactionContext
On 2013-10-07 15:02:36 -0400, Robert Haas wrote: On Fri, Oct 4, 2013 at 3:20 PM, Andres Freund and...@2ndquadrant.com wrote: On 2013-10-04 15:15:36 -0400, Robert Haas wrote: Andres, are you (or is anyone) going to try to fix this assertion failure? I think short term replacing it by IsTransactionOrTransactionBlock() is the way to go. Entirely restructuring how cache invalidation in the abort path works is not a small task. Well, if we're going to go that route, how about something like the attached? I included the assert-change per se, an explanatory comment, and the test case that Noah devised to cause the current assertion to fail. Sounds good to me. Maybe add a comment to the added regression test explaining it tests invalidation processing at xact abort? 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: [HACKERS] Re: custom hash-based COUNT(DISTINCT) aggregate - unexpectedly high memory consumption
Hi Atri! On 7.10.2013 16:56, Atri Sharma wrote: 3. Consider dropping buckets in favor of open addressing (linear probing, quadratic, whatever). This avoids another level of pointer indirection. OK, this sounds really interesting. It should be fairly straightforward for fixed-length data types (in that case I can get rid of the pointers altogether). Consider the aspects associated with open addressing.Open addressing can quickly lead to growth in the main table.Also, chaining is a much cleaner way of collision resolution,IMHO. What do you mean by growth in the main table? Tomas -- 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] space reserved for WAL record does not match what was written: panic on windows
On 07.10.2013 23:47, Andres Freund wrote: On 2013-10-07 13:25:17 -0400, Robert Haas wrote: And does that indicate that intptr_t is the wrong type to be using here? No, I don't think so. intptr_t is defined to be a integer type to which you can cast a pointer, cast it back and still get the old value. On 32bit platforms it usually will be 32bit wide. All that's fine for the classic usages of TYPEALIGN where it's used on pointers or lengths of stuff stored in memory. Those will always fit in 32bit on a 32bit platform. But here we're using it on explicit 64bit types (XLogRecPtr). Yep. Now, you could argue that we should make it use 64bit math everywhere - but I think that might incur quite the price on some 32bit platforms. It's used in the tuple decoding stuff, that's quite the hot path in some workloads. So I guess it's either a separate macro, or we rewrite that piece of code to work slightly differently and work directly on the lenght or such. Committed what is pretty much David's original patch. Maybe we should add a StaticAssert ensuring the TYPEALIGN macro only gets passed 32bit types? I tried that, like this: --- a/src/include/c.h +++ b/src/include/c.h @@ -532,7 +532,7 @@ typedef NameData *Name; */ #define TYPEALIGN(ALIGNVAL,LEN) \ - (((intptr_t) (LEN) + ((ALIGNVAL) - 1)) ~((intptr_t) ((ALIGNVAL) - 1))) + ( StaticAssertExpr(sizeof(LEN) = 4, type is too wide), ((intptr_t) (LEN) + ((ALIGNVAL) - 1)) ~((intptr_t) ((ALIGNVAL) - 1))) #define SHORTALIGN(LEN)TYPEALIGN(ALIGNOF_SHORT, (LEN)) #define INTALIGN(LEN) TYPEALIGN(ALIGNOF_INT, (LEN)) However, it gave a lot of errors from places where we have something like char buf[MaxHeapTuplesPerPage]. MaxHeapTuplesPerPage uses MAXALIGN, and gcc doesn't like it when StaticAssertExpr is used in such a context (to determine the size of an array). I temporarily changed places where that was a problem to use a copy of TYPEALIGN with no assertion, to verify that there are no more genuine bugs like this lurking. It was worth it as a one-off check, but I don't think we want to commit that. Thanks for the report, and analysis! - Heikki -- 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] SSI freezing bug
On Mon, 2013-10-07 at 23:49 +0300, Heikki Linnakangas wrote: On 07.10.2013 23:45, Heikki Linnakangas wrote: On 07.10.2013 22:35, Kevin Grittner wrote: Kevin Grittnerkgri...@ymail.com wrote: There might be a problem if someone applies this fix while any prepared transactions are pending. Still investigating the impact and possible fixes. I found a one-line fix for this. It tested without problem. Good catch. To fix the bug that Hannu pointed out, we also need to apply these fixes: http://www.postgresql.org/message-id/52440266.5040...@vmware.com Per a chat with Bruce, I'm going to apply that patch now to get it into the minor releases. 9.1 branch is broken now. -- 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: Fix bugs in SSI tuple locking.
Heikki Linnakangas heikki.linnakan...@iki.fi wrote: Fix bugs in SSI tuple locking. Thanks Heikki, both for these fixes and the discovery and discussion of the xmin issue! -- Kevin Grittner EDB: 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: [HACKERS] Bugfix and new feature for PGXS
On Sun, 2013-09-29 at 19:09 -0400, Andrew Dunstan wrote: On 09/03/2013 04:04 AM, Cédric Villemain wrote: Simple one, attached. I didn't document USE_VPATH, not sure how to explain that clearly. Just a remember that the doc is written and is waiting to be commited. There is also an issue spoted by Christoph with the installdirs prerequisite, the attached patch fix that. I applied this one line version of the patch, which seemed more elegant than the later one supplied. I backpatched that and the rest of the VPATH changes for extensiuons to 9.1 where we first provided for extensions, so people can have a reasonably uniform build system for their extensions. I suspect this line submake-libpq: $(libdir)/libpq.so ; will cause problems on platforms with a different extension (e.g. OS X). Given that this supposedly small and noninvasive set of changes has caused a number of problems already, I suggest we revert this entire thing until we have had time to actually test it somewhere other than in the the stable branches. As it stands, it is a new feature being developed in the stable branches, which is clearly not acceptable. -- 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] SSI freezing bug
On 08.10.2013 03:25, Peter Eisentraut wrote: On Mon, 2013-10-07 at 23:49 +0300, Heikki Linnakangas wrote: To fix the bug that Hannu pointed out, we also need to apply these fixes: http://www.postgresql.org/message-id/52440266.5040...@vmware.com Per a chat with Bruce, I'm going to apply that patch now to get it into the minor releases. 9.1 branch is broken now Rats. I forgot to git add the latest changes while backpatching. Committed a fix for that. - Heikki -- 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] Bugfix and new feature for PGXS
On 10/07/2013 08:47 PM, Peter Eisentraut wrote: On Sun, 2013-09-29 at 19:09 -0400, Andrew Dunstan wrote: On 09/03/2013 04:04 AM, Cédric Villemain wrote: Simple one, attached. I didn't document USE_VPATH, not sure how to explain that clearly. Just a remember that the doc is written and is waiting to be commited. There is also an issue spoted by Christoph with the installdirs prerequisite, the attached patch fix that. I applied this one line version of the patch, which seemed more elegant than the later one supplied. I backpatched that and the rest of the VPATH changes for extensiuons to 9.1 where we first provided for extensions, so people can have a reasonably uniform build system for their extensions. I suspect this line submake-libpq: $(libdir)/libpq.so ; will cause problems on platforms with a different extension (e.g. OS X). Given that this supposedly small and noninvasive set of changes has caused a number of problems already, I suggest we revert this entire thing until we have had time to actually test it somewhere other than in the the stable branches. As it stands, it is a new feature being developed in the stable branches, which is clearly not acceptable. If you want to revert it then go ahead, but the last statement is simply incorrect. The code has been sitting in HEAD for several months, and I committed on the back branches because it was wanted. cheers andrew -- 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] Patch: FORCE_NULL option for copy COPY in CSV mode
On Tue, Oct 8, 2013 at 12:55 AM, Andrew Dunstan and...@dunslane.net wrote: On 10/07/2013 03:06 PM, Robert Haas wrote: Also if your use case is to treat empty strings as NULL (as per above documentation), can't it be handled with WITH NULL AS option. For example, something like: postgres=# COPY testnull FROM stdin with CSV NULL AS E''; Enter data to be copied followed by a newline. End with a backslash and a period on a line by itself. 50, \. postgres=# select * from testnull; a | b +-- 50 | NULL (1 row) Good point. If this patch is just implementing something that can already be done with another syntax, we don't need it. Isn't the point of this option to allow a *quoted* empty string to be forced to NULL? If so, this is not testing the same case - in fact the COPY command above just makes explicit the default CSV NULL setting anyway. I am really not sure if all the purpose of patch can be achieved by existing syntax, neither it is explained clearly. However the proposal hasn't discussed why it's not good idea to extend some similar syntax COPY .. NULL which is used to replace string with NULL's? Description of NULL says: Specifies the string that represents a null value. Now why can't this syntax be extended to support quoted empty string if it's not supported currently? I have not checked completely, If it's difficult or not possible to support in existing syntax, then even it add's more value to introduce new syntax. By asking above question, I doesn't mean that we should not go for the new proposed syntax, rather it's to know and understand the benefit of new syntax, also it helps during CF review for reviewer's if the proposal involves new syntax and that's discussed previously. 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] Re: custom hash-based COUNT(DISTINCT) aggregate - unexpectedly high memory consumption
Hi Tomas, Consider the aspects associated with open addressing.Open addressing can quickly lead to growth in the main table.Also, chaining is a much cleaner way of collision resolution,IMHO. What do you mean by growth in the main table? Sorry, I should have been more verbose. AFAIK, Open addressing can be slower with a load factor approaching 1 as compared to chaining. Also, I feel that implementation of open addressing can be more complicated as we have to deal with deletes etc. I feel we can redesign our current chaining mechanism to have skip lists instead of singly linked lists. I experimented with it sometime back and I feel that it gives a stable performance with higher loads. Regards, Atri -- Regards, Atri l'apprenant -- 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] Re: custom hash-based COUNT(DISTINCT) aggregate - unexpectedly high memory consumption
On Tue, Oct 8, 2013 at 1:23 AM, Atri Sharma atri.j...@gmail.com wrote: Consider the aspects associated with open addressing.Open addressing can quickly lead to growth in the main table.Also, chaining is a much cleaner way of collision resolution,IMHO. What do you mean by growth in the main table? Sorry, I should have been more verbose. AFAIK, Open addressing can be slower with a load factor approaching 1 as compared to chaining. Also, I feel that implementation of open addressing can be more complicated as we have to deal with deletes etc. Deletes for a hash aggregate? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
