[HACKERS] Unwanted LOG during recovery of DROP TABLESPACE REDO
I found and fixed a bug that causes recovery (crash recovery , PITR) to throw unwanted LOG message if the tablespace symlink is not found during the processing of DROP TABLESPACE redo. LOG: could not remove symbolic link pg_tblspc/16384: No such file or directory To Reproduce the issue: 1. Start the server. 2. Create a tablespace. 3. Perform Checkpoint. 4. Drop tablespace. 5. Stop server using immediate mode. 6. Start server : At this stage, recovery throw log message as mentioned above. Reason is that DROP TABLESPACE has already removed symlink and again it is being tried to remove during recovery. As it is very much possible that DROP TABLESPACE was successful and cleaned up the file before server crashed. So this should be considered as valid scenario and no need to throw any LOG in such case. In case of processing of CREATE TABLESPACE redo, same is already handled. I will add this to 2014-08 CF for review. Thanks and Regards, Kumar Rajeev Rastogi rec_issue_with_drop_tblspc_redo.patch Description: rec_issue_with_drop_tblspc_redo.patch -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Bug in spg_range_quad_inner_consistent for adjacent operator (was Re: [HACKERS] Add a filed to PageHeaderData)
On 07/16/2014 08:30 AM, Michael Paquier wrote: On Wed, Jul 16, 2014 at 1:34 PM, Pavan Deolasee pavan.deola...@gmail.com wrote: Heikki, did you get chance to commit your patch? IMHO we should get the bug fix in before minor releases next week. My apologies if you've already committed it and I've missed the commit message. FWIW, this patch has not been committed yet. I am not seeing any recent update on src/backend/utils/adt/rangetypes_spgist.c. Thanks for the reminder, committed now. - 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] TODO : Allow parallel cores to be used by vacuumdb [ WIP ]
On Jul 16, 2014 7:05 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Tom Lane wrote: Dilip kumar dilip.ku...@huawei.com writes: On 15 July 2014 19:01, Magnus Hagander Wrote, I am late to this game, but the first thing to my mind was - do we really need the whole forking/threading thing on the client at all? Thanks for the review, I understand you point, but I think if we have do this directly by independent connection, It's difficult to equally divide the jobs b/w multiple independent connections. That argument seems like complete nonsense. You're confusing work allocation strategy with the implementation technology for the multiple working threads. I see no reason why a good allocation strategy couldn't work with either approach; indeed, I think it would likely be easier to do some things *without* client-side physical parallelism, because that makes it much simpler to handle feedback between the results of different operational threads. So you would have one initial connection, which generates a task list; then open N libpq connections. Launch one vacuum on each, and then sleep on select() on the three sockets. Whenever one returns read-ready, the vacuuming is done and we send another item from the task list. Repeat until tasklist is empty. No need to fork anything. Yeah, those are exactly my points. I think it would be significantly simpler to do it that way, rather than forking and threading. And also easier to make portable... (and as a optimization on Alvaros suggestion, you can of course reuse the initial connection as one of the workers as long as you got the full list of tasks from it up front, which I think you do anyway in order to do sorting of tasks...) /Magnus
Re: [HACKERS] Deadlocks in HS (on 9.0 :( )
On 2014-07-15 16:54:05 +0100, Greg Stark wrote: We've observed a 9.0 database have undetected deadlocks repeatedly in hot standby mode. I think what's happening is that autovacuum is kicking off a VACUUM of some system catalogs -- it seems to usually be pg_statistics' toast table actually. At the end of the vacuum it briefly gets the exclusive lock to truncate the table. On the standby it replays that and records the exclusive lock being taken. It then sees a cleanup record that pauses replay because a HS standby transaction is running that can see the xid being cleaned up. That transaction then blocks against the exclusive lock and deadlocks against recovery. Hm. Does that resolve itself after max_standby_streaming_delay? Because I don't really see how it'd actually have an undetected deadlock in that case. 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] Built-in binning functions
On 08/07/14 02:14, Tom Lane wrote: Petr Jelinek p...@2ndquadrant.com writes: here is a patch implementing varwidth_bucket (naming is up for discussion) function which does binning with variable bucket width. I didn't see any discussion of the naming question in this thread. I'd like to propose that it should be just width_bucket(); we can easily determine which function is meant, considering that the SQL-spec variants don't take arrays and don't even have the same number of actual arguments. I did mention in submission that the names are up for discussion, I am all for naming it just width_bucket. So given plain integer arguments, we'll invoke the float8 version not the int8 version, which may be undesirable. The same for int2 arguments. We could fix that by adding bespoke int4 and maybe int2 variants, but Hmm, yeah I don't love the idea of having 3 functions just to cover integer datatypes :(. TBH, I'm not sure that the specific-type functions are worth the trouble. Maybe we should just have one generic function, and take the trouble to optimize its array-subscripting calculations for either fixed-length or variable-length array elements? Have you got performance measurements demonstrating that multiple implementations really buy enough to justify the extra code? The performance difference is about 20% (+/- few depending on the array size), I don't know if that's bad enough to warrant type-specific implementation. I personally don't know how to make the generic implementation much faster than it is now, except maybe by turning it into aggregate which would cache the deconstructed version of the array, but that change semantics quite a bit and is probably not all that desirable. Also, I'm not convinced by this business of throwing an error for a NULL array element. Per spec, null arguments to width_bucket() produce a null result, not an error --- shouldn't this flavor act similarly? In any case I think the test needs to use array_contains_nulls() not just ARR_HASNULL. I am not against returning NULL for NULL array, I would still like to error on array that has values + NULL somewhere in the middle though. -- 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
[HACKERS] [BUG?] tuples from file_fdw has strange xids.
Hello, I found that tuples come from file_fdw has strange xmin and xmax.
postgres=# select tableoid, xmin, xmax, * from passwd1;
tableoid | xmin |xmax| uname | pass | uid | gid | ..
16396 | 244 | 4294967295 | root | x| 0 | 0 | root...
16396 | 252 | 4294967295 | bin | x| 1 | 1 | bin...
16396 | 284 | 4294967295 | daemon| x| 2 | 2 |
daemon...
Back to 9.1 gives the same result.
These xmin and xmax are the simple interpretations of a
DatumTupleFields filled by ExecMaterializedSlot() beeing fed the
source virtual tuple slot from file_fdw. On the other hand,
postgres_fdw gives sane xids (xmin = 2, xmax = 0).
This is because ForeignNext returns physical tuples in which
their headers are DatumTupleFields regardless whether the system
columns are requested or not. The fdw machinary desn't seem to
provide fdw handlers with a means to reject requests for
unavailable system columns, so the tuple header should be fixed
with the sane values as HeapTupleFields.
The patch attached fixes the header of materialized tuple to be
sane (2, 0) if the source slot was a virtual tuple in mechanism().
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/executor/nodeForeignscan.c b/src/backend/executor/nodeForeignscan.c
index 9cc5345..59dc5f4 100644
--- a/src/backend/executor/nodeForeignscan.c
+++ b/src/backend/executor/nodeForeignscan.c
@@ -22,6 +22,8 @@
*/
#include postgres.h
+#include access/transam.h
+#include access/htup_details.h
#include executor/executor.h
#include executor/nodeForeignscan.h
#include foreign/fdwapi.h
@@ -58,8 +60,21 @@ ForeignNext(ForeignScanState *node)
*/
if (plan-fsSystemCol !TupIsNull(slot))
{
+ bool was_virtual_tuple = (slot-tts_tuple == NULL);
HeapTuple tup = ExecMaterializeSlot(slot);
+ if (was_virtual_tuple)
+ {
+ /*
+ * ExecMaterializeSlot fills the tuple header as a
+ * DatumTupleFields if the slot was a virtual tuple, although a
+ * physical one is needed by the callers. So rewrite the tuple
+ * header as a sane HeapTupleFields.
+ */
+ HeapTupleHeaderSetXmin(tup-t_data, FrozenTransactionId);
+ HeapTupleHeaderSetXmax(tup-t_data, InvalidTransactionId);
+ HeapTupleHeaderSetCmin(tup-t_data, FirstCommandId);
+ }
tup-t_tableOid = RelationGetRelid(node-ss.ss_currentRelation);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] BUFFER_LOCK_EXCLUSIVE is used in ginbuildempty().
Hello, As far as I see gin seems using GIN_EXCLUSIVE instead of BUFFER_LOCK_EXCLUSIVE for LockBuffer, but the raw BUFFER_LOCK_EXCLUSIVE appears in ginbuildempty(). Does it has a meaning to fix them to GIN_EXCLUSIVE? regards, -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/src/backend/access/gin/gininsert.c b/src/backend/access/gin/gininsert.c index b27cae3..004b3a9 100644 --- a/src/backend/access/gin/gininsert.c +++ b/src/backend/access/gin/gininsert.c @@ -442,10 +442,10 @@ ginbuildempty(PG_FUNCTION_ARGS) /* An empty GIN index has two pages. */ MetaBuffer = ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL); - LockBuffer(MetaBuffer, BUFFER_LOCK_EXCLUSIVE); + LockBuffer(MetaBuffer, GIN_EXCLUSIVE); RootBuffer = ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL); - LockBuffer(RootBuffer, BUFFER_LOCK_EXCLUSIVE); + LockBuffer(RootBuffer, GIN_EXCLUSIVE); /* Initialize and xlog metabuffer and root buffer. */ START_CRIT_SECTION(); -- 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] [bug fix] pg_ctl always uses the same event source
On Wed, Jul 16, 2014 at 6:37 AM, Amit Kapila amit.kapil...@gmail.com wrote: On Tue, Jul 15, 2014 at 8:57 PM, MauMau maumau...@gmail.com wrote: From: Magnus Hagander mag...@hagander.net Well, it does in a couple of places. I'm nto sure it's that important (as nobody has AFAIK ever requested that change from for example EDB), but it's not a bad thing. I think this is nothing specific to any vendor rather it's good to have a define when it is used at multiple places. Sure, I don't object to the change itself, but the motivation was strange. There's also the change to throw an error if the source is already registered, which is potentially a bigger problem. Since the default will be the same everywhere, do we really want to throw an error when you install a second version, now that this is the normal state? There's also definitely a problem in that that codepath fires up a MessageBox, but it's just a function called in a DLL. It might just as well be called from a background service or from within an installer with no visible desktop, at which point the process will appear to be hung... I'm pretty sure you're not allowed to do that. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.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] Allowing join removals for more join types
On Wed, Jul 16, 2014 at 1:17 PM, Tom Lane t...@sss.pgh.pa.us wrote: David Rowley dgrowle...@gmail.com writes: I've attached an updated patch which puts in some fast path code for subquery type joins. I'm really not too sure on a good name for this function. I've ended up with query_supports_distinctness() which I'm not that keen on, but I didn't manage to come up with anything better. I've committed this with some mostly but not entirely cosmetic changes. Great! thanks for taking the time to give me guidance on this and commit it too. Simon, thank you for taking the time to review the code. Notably, I felt that pathnode.c was a pretty questionable place to be exporting distinctness-proof logic from, and after some reflection decided to move those functions to analyzejoins.c; that's certainly a better place for them than pathnode.c, and I don't see any superior third alternative. That seems like a good change. Also makes be wonder a bit why clause_sides_match_join is duplicated in joinpath.c and analyzejoins.c, is this just so that it can be inlined? Thanks also for making the change to create_unique_path to make use of the new query_supports_distinctness function. Regards David Rowley
[HACKERS] [TODO] Process pg_hba.conf keywords as case-insensitive
Attached patch is implementing following TODO item Process pg_hba.conf keywords as case-insensitive * More robust pg_hba.conf parsing/error logginghttp://archives.postgresql.org/pgsql-hackers/2009-09/msg00432.php Thanks Regards, Viswanatham Kiran Kumar pg_hba.conf_keywords_as_case-insensitive.patch Description: pg_hba.conf_keywords_as_case-insensitive.patch -- 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] Allowing NOT IN to use ANTI joins
On Wed, Jul 16, 2014 at 9:11 AM, Tom Lane t...@sss.pgh.pa.us wrote: Simon Riggs si...@2ndquadrant.com writes: On 15 July 2014 12:58, David Rowley dgrowle...@gmail.com wrote: I found that the call to is_NOTANY_compatible_with_antijoin adds about 0.2% and 2.3% to total planning time. Though the 2.3% was quite an extreme case, and the 0.2% was the most simple case I could think of. Is there a way we can only run this extra test when we have reasonable idea that there is potential to save significant costs? Well, all of this cost occurs only when we find a NOT IN clause in a place where we could conceivably turn it into an antijoin. I think it's unquestionably a win to make that transformation if possible. My concern about it is mainly that the whole thing is a band-aid for naively written queries, and it seems likely to me that naively written queries aren't going to be amenable to making the proof we need. But we can't tell that for sure without doing exactly the work the patch does. I do think Simon has a good point, maybe it's not something for this patch, but perhaps other planner patches that potentially optimise queries that could have been executed more efficiently if they had been written in another way. Since the planning time has been added to EXPLAIN ANALYZE I have noticed that in many very simple queries that quite often planning time is longer than execution time, so I really do understand the concern that we don't want to slow the planner down for these super fast queries. But on the other hand, if this extra 1-2 microseconds that the NOT IN optimisation was being frowned upon and the patch had been rejected based on that, at the other end of the scale, I wouldn't think it was too impossible for spending that extra 2 microseconds to translate into shaving a few hours off of the execution time of a query being run in an OLAP type workload. If that was the case then having not spent the 2 extra microseconds seems quite funny, but there's no real way to tell I guess unless we invented something to skip the known costly optimisations on first pass then re-plan the whole query with the planning strength knob turned to the maximum if the final query cost more than N. Off course such a idea would make bad plans even harder to debug, so it's far from perfect, but I'm not seeing other options for the best of both worlds. Regards David Rowley
Re: [HACKERS] better atomics - v0.5
On Mon, Jul 14, 2014 at 12:47 AM, Andres Freund and...@2ndquadrant.com
wrote:
On 2014-07-08 11:51:14 +0530, Amit Kapila wrote:
On Thu, Jul 3, 2014 at 10:56 AM, Amit Kapila amit.kapil...@gmail.com
wrote:
Further review of patch:
1.
/*
* pg_atomic_test_and_set_flag - TAS()
*
* Acquire/read barrier semantics.
*/
STATIC_IF_INLINE_DECLARE bool
pg_atomic_test_set_flag(volatile pg_atomic_flag *ptr);
a. I think Acquire and read barrier semantics aren't equal.
Right. It was mean as Acquire (thus including read barrier) semantics.
Okay, I got confused by reading comments, may be saying the same
explicitly in comments is better.
I guess I'll better update README.barrier to have definitions of all
barriers.
I think updating about the reasons behind using specific
barriers for atomic operations defined by PG can be useful
for people who want to understand or use the atomic op API's.
b. I think it adheres to Acquire semantics for platforms where
that semantics
are supported else it defaults to full memory ordering.
Example :
#define TAS(lock) (InterlockedCompareExchange(lock, 1, 0))
Is it right to declare that generic version of function adheres to
Acquire semantics.
Implementing stronger semantics than required should always be
fine. There's simply no sane way to work with the variety of platforms
we support in any other way.
Agreed, may be we can have a note somewhere either in code or
readme to mention about it, if you think that can be helpful.
2.
bool
pg_atomic_test_set_flag_impl(volatile pg_atomic_flag *ptr)
{
return TAS((slock_t *) ptr-sema);
#define TAS(lock) (InterlockedCompareExchange(lock, 1, 0))
Where's that from?
Its there in s_lock.h. Refer below code:
#ifdef WIN32_ONLY_COMPILER
typedef LONG slock_t;
#define HAS_TEST_AND_SET
#define TAS(lock) (InterlockedCompareExchange(lock, 1, 0))
I can't recall adding a line of code like that?
It is not added by your patch but used in your patch.
I am not sure if that is what excatly you want atomic API to define
for WIN32, but I think it is picking this definition. I have one question
here, if the value of sema is other than 1 or 0, then it won't set the flag
and not sure what it will return (ex. if the value is negative), so is this
implementation completely sane?
a. In above usage (ptr-sema), sema itself is not declared as volatile,
so would it be right to use it in this way for API
(InterlockedCompareExchange)
expecting volatile.
Upgrading a pointer to volatile is defined, so I don't see the problem?
Thats okay. However all other structure definitions use it as
volatile, example for atomics-arch-amd64.h it is defined as follows:
#define PG_HAVE_ATOMIC_FLAG_SUPPORT
typedef struct pg_atomic_flag
{
volatile char value;
} pg_atomic_flag;
So is there any harm if we keep this definition in sync with others?
3.
/*
* pg_atomic_unlocked_test_flag - TAS()
*
* No barrier semantics.
*/
STATIC_IF_INLINE_DECLARE bool
pg_atomic_unlocked_test_flag(volatile pg_atomic_flag *ptr);
a. There is no setting in this, so using TAS in function comments
seems to be wrong.
Good point.
b. BTW, I am not able see this function in C11 specs.
So? It's needed for a sensible implementation of spinlocks ontop of
atomics.
Okay got the point, do you think it makes sense to have a common
agreement/consensus w.r.t which API's are necessary as a first
cut. For me as a reviewer, if the API is implemented as per specs or
what it is desired to then we can have it, however I am not sure if there
is general consensus or at least some people agreed to set of API's
which are required for first cut, have I missed some conclusion/discussion
about it.
5.
atomics-generic-gcc.h
#ifndef PG_HAS_ATOMIC_UNLOCKED_TEST_FLAG
#define PG_HAS_ATOMIC_UNLOCKED_TEST_FLAG
static inline bool
pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr)
{
return ptr-value == 0;
}
#endif
Don't we need to compare it with 1 instead of 0?
Why? It returns true if the lock is free, makes sense imo?
Other implementation in atomics-generic.h seems to implement it other
way
#define PG_HAS_ATOMIC_UNLOCKED_TEST_FLAG
static inline bool
pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr)
{
return pg_atomic_read_u32_impl(ptr) == 1;
}
Will add comments to atomics.h
I think that will be helpful.
6.
atomics-fallback.h
pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr)
{
/*
* Can't do this in the semaphore based implementation - always return
* true. Do this in the header so compilers can optimize the test away.
*/
return true;
}
Why we can't implement this in semaphore based implementation?
Because semaphores don't provide the API for it?
Okay, but it is not clear from comments why this test should always
return true, basically I am not able to think why incase of missing API
returning true is correct behaviour for this API?
Re: [HACKERS] SSL information view
On Mon, Jul 14, 2014 at 7:54 PM, Stefan Kaltenbrunner ste...@kaltenbrunner.cc wrote: On 07/13/2014 10:35 PM, Magnus Hagander wrote: On Sun, Jul 13, 2014 at 10:32 PM, Stefan Kaltenbrunner ste...@kaltenbrunner.cc wrote: On 07/12/2014 03:08 PM, Magnus Hagander wrote: As an administrator, I find that you fairly often want to know what your current connections are actually using as SSL parameters, and there is currently no other way than gdb to find that out - something we definitely should fix. Yeah that would be handy - however I often wish to be able to figure that out based on the logfile as well, any chance of getting these into connection-logging/log_line_prefix as well? We do already log some of it if you have enabled log_connections - protocol and cipher. Anything else in particular you'd be looking for - compression info? DN mostly, not sure I care about compression info... Compression fitted more neatly in with the format that was there now. I wonder if we shuold add a DETAIL field on that error message that has the DN in case there is a client certificate. Would that make sense? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.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] [bug fix] pg_ctl always uses the same event source
On Wed, Jul 16, 2014 at 2:11 PM, Magnus Hagander mag...@hagander.net wrote: On Wed, Jul 16, 2014 at 6:37 AM, Amit Kapila amit.kapil...@gmail.com wrote: There's also the change to throw an error if the source is already registered, which is potentially a bigger problem. I think generally if the s/w is installed/registered and we try to install it second time without un-installing previous one, it gives some notice indicating the same. Since the default will be the same everywhere, do we really want to throw an error when you install a second version, now that this is the normal state? Allowing second version to overwrite the first can also cause problems similar to what is reported in this thread, basically what if the second installation/registration is removed, won't it cause the first one to loose messages? There's also definitely a problem in that that codepath fires up a MessageBox, but it's just a function called in a DLL. There are other paths in the same code which already fires up MessageBox. It might just as well be called from a background service or from within an installer with no visible desktop, at which point the process will appear to be hung... I'm pretty sure you're not allowed to do that. So in that case shouldn't we get rid of MessageBox() or provide alternate way of logging from pgevent module. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] [bug fix] pg_ctl always uses the same event source
On Wed, Jul 16, 2014 at 12:31 PM, Amit Kapila amit.kapil...@gmail.com wrote: On Wed, Jul 16, 2014 at 2:11 PM, Magnus Hagander mag...@hagander.net wrote: On Wed, Jul 16, 2014 at 6:37 AM, Amit Kapila amit.kapil...@gmail.com wrote: There's also the change to throw an error if the source is already registered, which is potentially a bigger problem. I think generally if the s/w is installed/registered and we try to install it second time without un-installing previous one, it gives some notice indicating the same. Since the default will be the same everywhere, do we really want to throw an error when you install a second version, now that this is the normal state? Allowing second version to overwrite the first can also cause problems similar to what is reported in this thread, basically what if the second installation/registration is removed, won't it cause the first one to loose messages? It will, this is true. I'm not sure there's a good way around that given now Windows Event Logs work though, unless we restrict usage a lot (as in only one installation of postgres at a time for example). I thnk it's better to document what you need to do in a case like this (re-register the existing DLL). There's also definitely a problem in that that codepath fires up a MessageBox, but it's just a function called in a DLL. There are other paths in the same code which already fires up MessageBox. Ouch, I didn't realize that - I just looked at the patch. What's worse is it's probably written by me :) However, I'd say the one being added here is much more likely to fire under such circumstances. Of the existing ones, the only really likely case for them to fire is a permission denied case, and that will most likely only happen from an interactive session. That might be why we haven't seen it... It might just as well be called from a background service or from within an installer with no visible desktop, at which point the process will appear to be hung... I'm pretty sure you're not allowed to do that. So in that case shouldn't we get rid of MessageBox() or provide alternate way of logging from pgevent module. It's something we might want to consider, but let's consider that a separate patch. Actually, it surprises me that we haven't had an issue before. But I guess maybe the installers don't actually use regsvr32, but instead just registers it manually by sticking it in the registry? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.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] [bug fix] pg_ctl always uses the same event source
On Wed, Jul 16, 2014 at 4:06 PM, Magnus Hagander mag...@hagander.net wrote: On Wed, Jul 16, 2014 at 12:31 PM, Amit Kapila amit.kapil...@gmail.com wrote: On Wed, Jul 16, 2014 at 2:11 PM, Magnus Hagander mag...@hagander.net wrote: On Wed, Jul 16, 2014 at 6:37 AM, Amit Kapila amit.kapil...@gmail.com wrote: There's also the change to throw an error if the source is already registered, which is potentially a bigger problem. I think generally if the s/w is installed/registered and we try to install it second time without un-installing previous one, it gives some notice indicating the same. Since the default will be the same everywhere, do we really want to throw an error when you install a second version, now that this is the normal state? Allowing second version to overwrite the first can also cause problems similar to what is reported in this thread, basically what if the second installation/registration is removed, won't it cause the first one to loose messages? It will, this is true. I'm not sure there's a good way around that given now Windows Event Logs work though, unless we restrict usage a lot (as in only one installation of postgres at a time for example). I thnk it's better to document what you need to do in a case like this (re-register the existing DLL). Given that now user has a way to use separate names for event log messages, I think it is better not to change default behaviour and rather just document the same. There's also definitely a problem in that that codepath fires up a MessageBox, but it's just a function called in a DLL. There are other paths in the same code which already fires up MessageBox. Ouch, I didn't realize that - I just looked at the patch. What's worse is it's probably written by me :) However, I'd say the one being added here is much more likely to fire under such circumstances. Of the existing ones, the only really likely case for them to fire is a permission denied case, and that will most likely only happen from an interactive session. That might be why we haven't seen it... It might just as well be called from a background service or from within an installer with no visible desktop, at which point the process will appear to be hung... I'm pretty sure you're not allowed to do that. So in that case shouldn't we get rid of MessageBox() or provide alternate way of logging from pgevent module. It's something we might want to consider, but let's consider that a separate patch. Sure, that make sense. So as a conclusion, the left over items to be handled for patch are: 1. Remove the new usage related to use of same event source name for registration from pgevent. 2. Document the information to prevent loss of messages in some scenarios as explained above. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] [bug fix] pg_ctl always uses the same event source
From: Magnus Hagander mag...@hagander.net There's also the change to throw an error if the source is already registered, which is potentially a bigger problem. Since the default will be the same everywhere, do we really want to throw an error when you install a second version, now that this is the normal state? There's also definitely a problem in that that codepath fires up a MessageBox, but it's just a function called in a DLL. It might just as well be called from a background service or from within an installer with no visible desktop, at which point the process will appear to be hung... I'm pretty sure you're not allowed to do that. I got what you mean. I removed changes in pgevent.c except for the default name. I attached the revised patch. More importantly, isn't it wrong to claim it will only be used for register and unregister? If we get an early failure in start, for example, there are numerous codepaths that will end up calling write_stderr(), which will use the eventlog when running as a service. Shouldn't the -e parameter be moved under common options? Yes, you are right. -e is effective only if pg_ctl is invoked as a Windows service. So it is written at register mode. That is, -e specifies the event source used by the Windows service which is registered by pg_ctl register. Oh, right. I see what you mean now. That goes for all parameters though, including -D, and we don't specify those as register mode only, so I still think it's wrong to place it there. It is now grouped with all other parameters that we specifically *don't* write to the commandline of the service. Sorry, but I'm probably not understanding your comment. This may be due to my English capability. -e is effective only on Windows, so it is placed in section Options for Windows. And I could not find a section named Common options. -e is currently meangful only with register mode, so it is placed at register mode in Synopsis section. For example, -D is not attached to kill mode. Do you suggest that -e should be attached to all modes in Synopsis section, or -e should be placed in the section Options instead of Options for Windows? Regards MauMau pg_ctl_eventsrc_v11.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] Process pg_hba.conf keywords as case-insensitive
On Wed, Jul 16, 2014 at 6:23 PM, Viswanatham kirankumar viswanatham.kiranku...@huawei.com wrote: Attached patch is implementing following TODO item Process pg_hba.conf keywords as case-insensitive More robust pg_hba.conf parsing/error logging You should consider adding this patch to the next commit fest: https://commitfest.postgresql.org/action/commitfest_view?id=23 Regards, -- 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] [bug fix] pg_ctl always uses the same event source
From: Amit Kapila amit.kapil...@gmail.com So as a conclusion, the left over items to be handled for patch are: 1. Remove the new usage related to use of same event source name for registration from pgevent. 2. Document the information to prevent loss of messages in some scenarios as explained above. I noticed the continued discussion after I had prepared and submitted the revised patch. OK, how about the patch in the previous mail, Magnus-san? Regards MauMau -- 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 : Allow parallel cores to be used by vacuumdb [ WIP ]
On 16 July 2014 12:13 Magnus Hagander Wrote, Yeah, those are exactly my points. I think it would be significantly simpler to do it that way, rather than forking and threading. And also easier to make portable... (and as a optimization on Alvaros suggestion, you can of course reuse the initial connection as one of the workers as long as you got the full list of tasks from it up front, which I think you do anyway in order to do sorting of tasks...) Oh, I got your point, I will update my patch and send, Now we can completely remove vac_parallel.h file and no need of refactoring also:) Thanks Regards, Dilip Kumar From: Magnus Hagander [mailto:mag...@hagander.net] Sent: 16 July 2014 12:13 To: Alvaro Herrera Cc: Dilip kumar; Jan Lentfer; Tom Lane; PostgreSQL-development; Sawada Masahiko; Euler Taveira Subject: Re: [HACKERS] TODO : Allow parallel cores to be used by vacuumdb [ WIP ] On Jul 16, 2014 7:05 AM, Alvaro Herrera alvhe...@2ndquadrant.commailto:alvhe...@2ndquadrant.com wrote: Tom Lane wrote: Dilip kumar dilip.ku...@huawei.commailto:dilip.ku...@huawei.com writes: On 15 July 2014 19:01, Magnus Hagander Wrote, I am late to this game, but the first thing to my mind was - do we really need the whole forking/threading thing on the client at all? Thanks for the review, I understand you point, but I think if we have do this directly by independent connection, It's difficult to equally divide the jobs b/w multiple independent connections. That argument seems like complete nonsense. You're confusing work allocation strategy with the implementation technology for the multiple working threads. I see no reason why a good allocation strategy couldn't work with either approach; indeed, I think it would likely be easier to do some things *without* client-side physical parallelism, because that makes it much simpler to handle feedback between the results of different operational threads. So you would have one initial connection, which generates a task list; then open N libpq connections. Launch one vacuum on each, and then sleep on select() on the three sockets. Whenever one returns read-ready, the vacuuming is done and we send another item from the task list. Repeat until tasklist is empty. No need to fork anything. Yeah, those are exactly my points. I think it would be significantly simpler to do it that way, rather than forking and threading. And also easier to make portable... (and as a optimization on Alvaros suggestion, you can of course reuse the initial connection as one of the workers as long as you got the full list of tasks from it up front, which I think you do anyway in order to do sorting of tasks...) /Magnus
[HACKERS] Improvement of versioning on Windows, take two
Hi all,
Please find attached a patch finishing the work begun during CF1. This
adds versioning support for all the remaining dll and exe files on
both MinGW and MSVC:
- regress.dll (MSVC only)
- isolationtester.exe
- pg_isolation_regress.exe
- pg_regress.exe
- pg_regress_ecpg.exe
- zic.exe
I will add this patch to CF2. Comments are welcome.
Regards,
Michael
From cb2d50e4f457d01b0f48f75d698b3b2fa23efa6f Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Wed, 16 Jul 2014 23:13:42 +0900
Subject: [PATCH] Versioning support on MinGW and MSVC for remaining exe and
dll files
Windows versioning is added for the following files:
- regress.dll (MSVC only)
- isolationtester.exe
- pg_isolation_regress.exe
- pg_regress.exe
- pg_regress_ecpg.exe
- zic.exe
---
src/interfaces/ecpg/test/Makefile | 1 +
src/test/isolation/Makefile | 7 +--
src/test/regress/GNUmakefile | 7 +--
src/timezone/Makefile | 5 -
src/tools/msvc/Mkvcbuild.pm | 6 ++
src/tools/msvc/clean.bat | 3 +++
6 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/src/interfaces/ecpg/test/Makefile b/src/interfaces/ecpg/test/Makefile
index 56f6a17..a2e0a83 100644
--- a/src/interfaces/ecpg/test/Makefile
+++ b/src/interfaces/ecpg/test/Makefile
@@ -14,6 +14,7 @@ override CPPFLAGS := \
$(CPPFLAGS)
PGFILEDESC = ECPG Test - regression tests for ECPG
+PGAPPICON = win32
# where to find psql for testing an existing installation
PSQLDIR = $(bindir)
diff --git a/src/test/isolation/Makefile b/src/test/isolation/Makefile
index 26bcf22..77bc43d 100644
--- a/src/test/isolation/Makefile
+++ b/src/test/isolation/Makefile
@@ -6,12 +6,15 @@ subdir = src/test/isolation
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
+PGFILEDESC = pg_isolation_tester/isolationtester - isolation regression tests
+PGAPPICON = win32
+
# where to find psql for testing an existing installation
PSQLDIR = $(bindir)
override CPPFLAGS := -I$(srcdir) -I$(libpq_srcdir) -I$(srcdir)/../regress $(CPPFLAGS)
-OBJS = specparse.o isolationtester.o
+OBJS = specparse.o isolationtester.o $(WIN32RES)
all: isolationtester$(X) pg_isolation_regress$(X)
@@ -21,7 +24,7 @@ submake-regress:
pg_regress.o: | submake-regress
rm -f $@ $(LN_S) $(top_builddir)/src/test/regress/pg_regress.o .
-pg_isolation_regress$(X): isolation_main.o pg_regress.o
+pg_isolation_regress$(X): isolation_main.o pg_regress.o $(WIN32RES)
$(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
isolationtester$(X): $(OBJS) | submake-libpq submake-libpgport
diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile
index b084e0a..64c9924 100644
--- a/src/test/regress/GNUmakefile
+++ b/src/test/regress/GNUmakefile
@@ -14,6 +14,9 @@ subdir = src/test/regress
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
+PGFILEDESC = pg_regress - regression tests
+PGAPPICON = win32
+
# file with extra config for temp build
TEMP_CONF =
ifdef TEMP_CONFIG
@@ -43,7 +46,7 @@ EXTRADEFS = '-DHOST_TUPLE=$(host_tuple)' \
all: pg_regress$(X)
-pg_regress$(X): pg_regress.o pg_regress_main.o | submake-libpgport
+pg_regress$(X): pg_regress.o pg_regress_main.o $(WIN32RES) | submake-libpgport
$(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
# dependencies ensure that path changes propagate
@@ -177,7 +180,7 @@ bigcheck: all tablespace-setup
clean distclean maintainer-clean: clean-lib
# things built by `all' target
rm -f $(OBJS) refint$(DLSUFFIX) autoinc$(DLSUFFIX) dummy_seclabel$(DLSUFFIX)
- rm -f pg_regress_main.o pg_regress.o pg_regress$(X)
+ rm -f pg_regress_main.o pg_regress.o pg_regress$(X) $(WIN32RES)
# things created by various check targets
rm -f $(output_files) $(input_files)
rm -rf testtablespace
diff --git a/src/timezone/Makefile b/src/timezone/Makefile
index ef739e9..ab65d22 100644
--- a/src/timezone/Makefile
+++ b/src/timezone/Makefile
@@ -12,11 +12,14 @@ subdir = src/timezone
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
+PGFILEDESC = timezone - timezone database
+PGAPPICON = win32
+
# files to build into backend
OBJS= localtime.o strftime.o pgtz.o
# files needed to build zic utility program
-ZICOBJS= zic.o ialloc.o scheck.o localtime.o
+ZICOBJS= zic.o ialloc.o scheck.o localtime.o $(WIN32RES)
# timezone data files
TZDATA = africa antarctica asia australasia europe northamerica southamerica \
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index b71da67..d5a0b3b 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -329,6 +329,7 @@ sub mkvcbuild
$pgregress_ecpg-AddIncludeDir('src\test\regress');
$pgregress_ecpg-AddDefine('HOST_TUPLE=i686-pc-win32vc');
$pgregress_ecpg-AddDefine('FRONTEND');
+ $pgregress_ecpg-AddResourceFile('src\interfaces\ecpg\test');
$pgregress_ecpg-AddReference($libpgcommon, $libpgport);
my $isolation_tester =
@@ -344,6 +345,7 @@ sub mkvcbuild
Re: [HACKERS] Pg_upgrade and toast tables bug discovered
On Mon, Jul 14, 2014 at 09:40:33PM +0200, Andres Freund wrote: On 2014-07-11 09:55:34 -0400, Bruce Momjian wrote: On Fri, Jul 11, 2014 at 09:48:06AM -0400, Bruce Momjian wrote: Uh, why does this need to be in ALTER TABLE? Can't this be part of table creation done by pg_dump? Uh, I think you need to read the thread. We have to delay the toast creation part so we don't use an oid that will later be required by another table from the old cluster. This has to be done after all tables have been created. We could have pg_dump spit out those ALTER lines at the end of the dump, but it seems simpler to do it in pg_upgrade. Even if we have pg_dump create all the tables that require pre-assigned TOAST oids first, then the other tables that _might_ need a TOAST table, those later tables might create a toast oid that matches a later non-TOAST-requiring table, so I don't think that fixes the problem. What would be nice is if I could mark just the tables that will need toast tables created in that later phase (those tables that didn't have a toast table in the old cluster, but need one in the new cluster). However, I can't see where to store that or how to pass that back into pg_upgrade. I don't see a logical place in pg_class to put it. This seems overengineered. Why not just do SELECT pg_upgrade.maintain_toast(oid) FROM pg_class WHERE relkind = 'r'; and in maintain_toast() just call AlterTableCreateToastTable()? I didn't think you could call into backend functions like that, and if I did, it might break something in the future. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] Pg_upgrade and toast tables bug discovered
On 2014-07-16 10:19:05 -0400, Bruce Momjian wrote: What would be nice is if I could mark just the tables that will need toast tables created in that later phase (those tables that didn't have a toast table in the old cluster, but need one in the new cluster). However, I can't see where to store that or how to pass that back into pg_upgrade. I don't see a logical place in pg_class to put it. This seems overengineered. Why not just do SELECT pg_upgrade.maintain_toast(oid) FROM pg_class WHERE relkind = 'r'; and in maintain_toast() just call AlterTableCreateToastTable()? I didn't think you could call into backend functions like that, and if I did, it might break something in the future. Why not? Pg_upgrade is already intimately linked into to the way the backend works. 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] Unwanted LOG during recovery of DROP TABLESPACE REDO
Rajeev rastogi rajeev.rast...@huawei.com writes: I found and fixed a bug that causes recovery (crash recovery , PITR) to throw unwanted LOG message if the tablespace symlink is not found during the processing of DROP TABLESPACE redo. LOG: could not remove symbolic link pg_tblspc/16384: No such file or directory I don't think that's a bug: it's the designed behavior. Why should we complicate the code to not print a log message in a situation where it's unclear if the case is expected or not? 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] Allowing join removals for more join types
David Rowley dgrowle...@gmail.com writes: On Wed, Jul 16, 2014 at 1:17 PM, Tom Lane t...@sss.pgh.pa.us wrote: Notably, I felt that pathnode.c was a pretty questionable place to be exporting distinctness-proof logic from, and after some reflection decided to move those functions to analyzejoins.c; that's certainly a better place for them than pathnode.c, and I don't see any superior third alternative. That seems like a good change. Also makes be wonder a bit why clause_sides_match_join is duplicated in joinpath.c and analyzejoins.c, is this just so that it can be inlined? Hm ... probably just didn't seem worth the trouble to try to share the code. It's not really something that either module should be exporting. I guess some case could be made for exporting it from util/restrictinfo.c, but it'd still seem like a bit of a wart. 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] [BUG?] tuples from file_fdw has strange xids.
Kyotaro HORIGUCHI horiguchi.kyot...@lab.ntt.co.jp writes: Hello, I found that tuples come from file_fdw has strange xmin and xmax. file_fdw isn't documented to return anything useful for xmin/xmax/etc, so I don't find this surprising. The patch attached fixes the header of materialized tuple to be sane (2, 0) if the source slot was a virtual tuple in mechanism(). I don't really think it's ForeignNext's place to be doing something about this. It seems like a useless expenditure of cycles. Besides, this fails to cover e.g. postgres_fdw, which is likewise unconcerned about what it returns for system columns other than ctid. 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] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Re: Fabrízio de Royes Mello 2014-07-15
CAFcNs+pXpmfwi_rKF-cSBOHWC+E=xtsrnxicrgay6bcmthb...@mail.gmail.com
What about the wqueue mechanism, though? Isn't that made exactly for
the kind of catalog updates you are doing?
Works, but this mechanism create a new entry in pg_class for toast, so it's
a little different than use the cluster_rel that generate a new
relfilenode. The important is both mechanisms create new datafiles.
Ok, I had thought that any catalog changes in AT should be queued
using this mechanism to be executed later by ATExecCmd(). The queueing
only seems to be used for the cases that recurse into child tables,
which we don't.
Merged both to a single ATPrepSetLoggedOrUnlogged(Relation rel, bool
toLogged);
Thanks.
But they aren't duplicated... the first opens con-confrelid and the
other opens con-conrelid according toLogged branch.
Oh sorry. I had looked for that, but still missed it.
I removed because they are not so useful than I was thinking before.
Actually they just bloated our test cases.
Nod.
I think the tests could also use a bit more comments, notably the
commands that are expected to fail. So far I haven't tried to read
them but trusted that they did the right thing. (Though with the
SELECTs removed, it's pretty readable now.)
Added some comments.
Thanks, looks nice now.
+SET TABLESPACE replaceable
class=PARAMETERnew_tablespace/replaceable
RESET ( replaceable class=PARAMETERstorage_parameter/replaceable
[, ... ] )
INHERIT replaceable class=PARAMETERparent_table/replaceable
NO INHERIT replaceable class=PARAMETERparent_table/replaceable
OF replaceable class=PARAMETERtype_name/replaceable
NOT OF
OWNER TO replaceable class=PARAMETERnew_owner/replaceable
-SET TABLESPACE replaceable
class=PARAMETERnew_tablespace/replaceable
That should get a footnote in the final commit message.
@@ -2857,6 +2860,8 @@ AlterTableGetLockLevel(List *cmds)
case AT_AddIndexConstraint:
case AT_ReplicaIdentity:
case AT_SetNotNull:
+ case AT_SetLogged:
+ case AT_SetUnLogged:
cmd_lockmode = AccessExclusiveLock;
break;
@@ -3248,6 +3253,13 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd
*cmd,
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
+ case AT_SetLogged:
+ case AT_SetUnLogged:
+ ATSimplePermissions(rel, ATT_TABLE);
+ ATPrepSetLoggedOrUnlogged(rel, (cmd-subtype ==
AT_SetLogged)); /* SET {LOGGED | UNLOGGED} */
+ pass = AT_PASS_MISC;
+ tab-rewrite = true;
+ break;
default:/* oops */
elog(ERROR, unrecognized alter table type: %d,
(int) cmd-subtype);
@@ -3529,6 +3541,12 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
Relation rel,
case AT_GenericOptions:
ATExecGenericOptions(rel, (List *) cmd-def);
break;
+ case AT_SetLogged:
+ AlterTableSetLoggedOrUnlogged(rel, true);
+ break;
+ case AT_SetUnLogged:
+ AlterTableSetLoggedOrUnlogged(rel, false);
+ break;
default:/* oops */
elog(ERROR, unrecognized alter table type: %d,
(int) cmd-subtype);
I'd move all these next to the AT_DropCluster things like in all the
other lists.
/*
+ * ALTER TABLE name SET {LOGGED | UNLOGGED}
+ *
+ * Change the table persistence type from unlogged to permanent by
+ * rewriting the entire contents of the table and associated indexes
+ * into new disk files.
+ *
+ * The ATPrepSetLoggedOrUnlogged function check all precondictions
preconditions (without trailing space :)
+ * to perform the operation:
+ * - check if the target table is unlogged/permanente
permanent
+ * - check if not exists a foreign key to/from other unlogged/permanent
if no ... exists
+ */
+static void
+ATPrepSetLoggedOrUnlogged(Relation rel, bool toLogged)
+{
+ charrelpersistence;
+
+ relpersistence = (toLogged) ? RELPERSISTENCE_UNLOGGED :
+ RELPERSISTENCE_PERMANENT;
+
+ /* check if is an unlogged or permanent relation */
+ if (rel-rd_rel-relpersistence != relpersistence)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg(table %s is not %s,
+ RelationGetRelationName(rel),
+
Re: [HACKERS] [TODO] Process pg_hba.conf keywords as case-insensitive
Re: Viswanatham kirankumar 2014-07-16 ec867def52699d4189b584a14baa7c2165440...@blreml504-mbx.china.huawei.com Attached patch is implementing following TODO item Process pg_hba.conf keywords as case-insensitive * More robust pg_hba.conf parsing/error logginghttp://archives.postgresql.org/pgsql-hackers/2009-09/msg00432.php Hmm. I see a case for accepting ALL (as in hosts.allow(5)), so +1 on that, but I don't think the other keywords like host and peer should be valid in upper case. Possibly things like MD5 and GSSAPI are naturally spelled in upper case, but I have my doubts about the rest. Christoph -- c...@df7cb.de | http://www.df7cb.de/ -- 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] Process pg_hba.conf keywords as case-insensitive
Christoph Berg c...@df7cb.de writes: Re: Viswanatham kirankumar 2014-07-16 ec867def52699d4189b584a14baa7c2165440...@blreml504-mbx.china.huawei.com Attached patch is implementing following TODO item Process pg_hba.conf keywords as case-insensitive Hmm. I see a case for accepting ALL (as in hosts.allow(5)), so +1 on that, but I don't think the other keywords like host and peer should be valid in upper case. I think the argument was that SQL users are accustomed to thinking that keywords are case-insensitive. It makes sense to me that we should adopt that same convention in pg_hba.conf. Re-reading the original thread, there was also concern about whether we should try to make quoting/casefolding behave more like it does in SQL, specifically for matching pg_hba.conf items to SQL identifiers (database and role names). This patch doesn't seem to have addressed that part of it, but I think we need to think those things through before we just do a blind s/strcmp/pg_strcasecmp/g. Otherwise we might find that we've added ambiguity that will give us trouble when we do try to fix that. 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] Pg_upgrade and toast tables bug discovered
On Wed, Jul 16, 2014 at 10:19 AM, Bruce Momjian br...@momjian.us wrote: On Mon, Jul 14, 2014 at 09:40:33PM +0200, Andres Freund wrote: On 2014-07-11 09:55:34 -0400, Bruce Momjian wrote: On Fri, Jul 11, 2014 at 09:48:06AM -0400, Bruce Momjian wrote: Uh, why does this need to be in ALTER TABLE? Can't this be part of table creation done by pg_dump? Uh, I think you need to read the thread. We have to delay the toast creation part so we don't use an oid that will later be required by another table from the old cluster. This has to be done after all tables have been created. We could have pg_dump spit out those ALTER lines at the end of the dump, but it seems simpler to do it in pg_upgrade. Even if we have pg_dump create all the tables that require pre-assigned TOAST oids first, then the other tables that _might_ need a TOAST table, those later tables might create a toast oid that matches a later non-TOAST-requiring table, so I don't think that fixes the problem. What would be nice is if I could mark just the tables that will need toast tables created in that later phase (those tables that didn't have a toast table in the old cluster, but need one in the new cluster). However, I can't see where to store that or how to pass that back into pg_upgrade. I don't see a logical place in pg_class to put it. This seems overengineered. Why not just do SELECT pg_upgrade.maintain_toast(oid) FROM pg_class WHERE relkind = 'r'; and in maintain_toast() just call AlterTableCreateToastTable()? I didn't think you could call into backend functions like that, and if I did, it might break something in the future. It would be impossible to write meaningful code in extensions if that were true. Yeah, it could break in the future, but it's not particularly likely, the regression tests should catch it if it happens, and it's no worse a risk here than in any other extension code which does this. -- 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] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Hi, I quickly looked at this patch and I think there's major missing pieces around buffer management and wal logging. a) Currently buffers that are in memory marked as permanent/non-permanent aren't forced out to disk/pruned from cache, not even when they're dirty. b) When converting from a unlogged to a logged table the relation needs to be fsynced. c) Currently a unlogged table changed into a logged one will be corrupted on a standby because its contents won't ever be WAL logged. 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] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On 2014-07-16 20:25:42 +0200, Andres Freund wrote: Hi, I quickly looked at this patch and I think there's major missing pieces around buffer management and wal logging. a) Currently buffers that are in memory marked as permanent/non-permanent aren't forced out to disk/pruned from cache, not even when they're dirty. b) When converting from a unlogged to a logged table the relation needs to be fsynced. c) Currently a unlogged table changed into a logged one will be corrupted on a standby because its contents won't ever be WAL logged. Forget that, didn't notice that you're setting tab-rewrite = true. 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] Built-in binning functions
2014-07-16 10:04 GMT+02:00 Petr Jelinek p...@2ndquadrant.com: On 08/07/14 02:14, Tom Lane wrote: Petr Jelinek p...@2ndquadrant.com writes: here is a patch implementing varwidth_bucket (naming is up for discussion) function which does binning with variable bucket width. I didn't see any discussion of the naming question in this thread. I'd like to propose that it should be just width_bucket(); we can easily determine which function is meant, considering that the SQL-spec variants don't take arrays and don't even have the same number of actual arguments. I did mention in submission that the names are up for discussion, I am all for naming it just width_bucket. I had this idea too - but I am not sure if it is good idea. A distance between ANSI SQL with_bucket and our enhancing is larger than in our implementation of median for example. I can live with both names, but current name I prefer. So given plain integer arguments, we'll invoke the float8 version not the int8 version, which may be undesirable. The same for int2 arguments. We could fix that by adding bespoke int4 and maybe int2 variants, but Hmm, yeah I don't love the idea of having 3 functions just to cover integer datatypes :(. TBH, I'm not sure that the specific-type functions are worth the trouble. Maybe we should just have one generic function, and take the trouble to optimize its array-subscripting calculations for either fixed-length or variable-length array elements? Have you got performance measurements demonstrating that multiple implementations really buy enough to justify the extra code? The performance difference is about 20% (+/- few depending on the array size), I don't know if that's bad enough to warrant type-specific implementation. I personally don't know how to make the generic implementation much faster than it is now, except maybe by turning it into aggregate which would cache the deconstructed version of the array, but that change semantics quite a bit and is probably not all that desirable. I am not sure if our API is enough to do it - there are no any simple support for immutable parameters. The performance is one point. Second point is wrong result, when input array is not well sorted. But this check means next performance penalization. But we cannot do it. Also, I'm not convinced by this business of throwing an error for a NULL array element. Per spec, null arguments to width_bucket() produce a null result, not an error --- shouldn't this flavor act similarly? In any case I think the test needs to use array_contains_nulls() not just ARR_HASNULL. I am not against returning NULL for NULL array, I would still like to error on array that has values + NULL somewhere in the middle though. +1 Pavel -- 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
[HACKERS] Allow GRANT TRIGGER privilege to DROP TRIGGER (Re: Bug ##7716)
I'd reported a few years ago what seemed an inconsistency with the TRIGGER permission on tables where it allows a role to create a trigger but not to drop it. I don't have the original email to reply directly to the thread anymore, but I've moved on to actually trying to fix it myself, hence sending to this list instead. Original thread - http://www.postgresql.org/message-id/e1tead4-0004le...@wrigleys.postgresql.org I found RemoveTriggerById() in backend/commands/trigger.c which seems to be the code that actually drops the trigger. And I see in CreateTrigger() above it how to use pg_class_aclcheck() to check for valid permissions, so I was going to see about adding that code to the Remove section. However, I see no code in Remove for checking for object ownership, so I'm not sure how that is being enforced currently which would also have to be adjusted. I see down in RangeVarCallbackForRenameTrigger() that it uses pg_class_ownercheck() to do so, but can't find where that is being done for dropping a trigger. I've tried tracing the function calls in RemoveTriggerById() to see if one of them is doing the owner check, but I haven't been able to see it. The only other reference to RemoveTriggerById() I can find is in backend/catalog/dependency.c and I don't see the check there either. This is my first time really digging into the postgres core code to try and adjust something myself. Even if there's no interest in accepting a patch for this, I would appreciate some guidance on how to go about it. Thanks! -- Keith Fiske Database Administrator OmniTI Computer Consulting, Inc. http://www.keithf4.com
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Wed, Jul 16, 2014 at 3:53 PM, Andres Freund and...@2ndquadrant.com wrote: On 2014-07-16 20:25:42 +0200, Andres Freund wrote: Hi, I quickly looked at this patch and I think there's major missing pieces around buffer management and wal logging. a) Currently buffers that are in memory marked as permanent/non-permanent aren't forced out to disk/pruned from cache, not even when they're dirty. b) When converting from a unlogged to a logged table the relation needs to be fsynced. c) Currently a unlogged table changed into a logged one will be corrupted on a standby because its contents won't ever be WAL logged. Forget that, didn't notice that you're setting tab-rewrite = true. :-) -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello
[HACKERS] Question about src/timezone/zic.c
I've recently found some time on my hands and have decided to see about
contributing to the PostgreSQL project, so I've started browsing the code
to get somewhat familiar with it.
The file src/timezone/zic.c caused me to raise my eyebrows a bit and I have
a question to ask because of this.
I first noticed some loops dealing with the time that I suspect could be
replaced with straight line code calling the julian date conversions. But
in attempting to understand exactly what the loops were doing, I saw some
calls to eitol() which is the function that I'm really questioning. The
code for eitol() is as follows:
static long
eitol(int i)
{
longl;
l = i;
if ((i 0 l = 0) || (i == 0 l != 0) || (i 0 l = 0))
{
(void) fprintf(stderr,
_(%s: %d did not sign extend correctly\n),
progname, i);
exit(EXIT_FAILURE);
}
return l;
}
As you can see, all that function does is perform a length extension of a
signed int to a signed long. Additionally, it has a fair amount of code to
verify that the sign extension was properly done. Since the sign extension
is actually performed via the simple l=i; statement towards the beginning
of the function, it's rather obvious that we're relying upon the compiler
to perform the sign extension. Additionally, since this function doesn't
have any recovery if an invalid extension is performed, it means that if a
faulty compiler is encountered, PostgreSQL will always fail to operate the
instant any timezone computation is performed.
This raises the question of Does anyone on this mailing list know of any C
compiler that fails to properly sign extend an assignment of an int to a
long? Or to put it another way, Are there any C compilers that fail to
properly perform integer promotion from int to long?
As things stand, it looks to me like that function eitol() can be simply
deleted and the 22 calls to that function also removed. Shorter,
simpler,faster code is always a good thing after all.
Thank you for reading,
John Cochran
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On 2014-07-16 20:53:06 +0200, Andres Freund wrote: On 2014-07-16 20:25:42 +0200, Andres Freund wrote: Hi, I quickly looked at this patch and I think there's major missing pieces around buffer management and wal logging. a) Currently buffers that are in memory marked as permanent/non-permanent aren't forced out to disk/pruned from cache, not even when they're dirty. b) When converting from a unlogged to a logged table the relation needs to be fsynced. c) Currently a unlogged table changed into a logged one will be corrupted on a standby because its contents won't ever be WAL logged. Forget that, didn't notice that you're setting tab-rewrite = true. So, while that danger luckily isn't there I think there's something similar. Consider: CREATE TABLE blub(...); INSERT INTO blub ...; BEGIN; ALTER TABLE blub SET UNLOGGED; ROLLBACK; The rewrite will read in the 'old' contents - but because it's done after the pg_class.relpersistence is changed they'll all not be marked as BM_PERMANENT in memory. Then the ALTER TABLE is rolled back, including the relpersistence setting. Which will unfortunately leave pages with the wrong persistency setting in memory, right? 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] Question about src/timezone/zic.c
John Cochran j69coch...@gmail.com writes: [ useless code in zic.c ] I agree with you that that function looks pretty pointless, but here's the thing: that's not our code. Most of the code in src/timezone is taken directly from the IANA tzcode distribution (see src/timezone/README), and we're not very interested in diverging further from that upstream than we have to. Doing so would just complicate merging future upstream fixes. So if you want to pursue the question of whether eitol is worth its weight, the right thing to do would be to ask about it on the upstream mailing list (looks like tz at iana.org is the current list name). One thing that *would* be worth doing is merging upstream's recent updates; for all I know they've already dealt with this. According to the README, we last synced with the 2010c tzcode release, so we're short a few years worth of upstream bug fixes there. 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] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Wed, Jul 16, 2014 at 7:26 PM, Andres Freund and...@2ndquadrant.com wrote: On 2014-07-16 20:53:06 +0200, Andres Freund wrote: On 2014-07-16 20:25:42 +0200, Andres Freund wrote: Hi, I quickly looked at this patch and I think there's major missing pieces around buffer management and wal logging. a) Currently buffers that are in memory marked as permanent/non-permanent aren't forced out to disk/pruned from cache, not even when they're dirty. b) When converting from a unlogged to a logged table the relation needs to be fsynced. c) Currently a unlogged table changed into a logged one will be corrupted on a standby because its contents won't ever be WAL logged. Forget that, didn't notice that you're setting tab-rewrite = true. So, while that danger luckily isn't there I think there's something similar. Consider: CREATE TABLE blub(...); INSERT INTO blub ...; BEGIN; ALTER TABLE blub SET UNLOGGED; ROLLBACK; The rewrite will read in the 'old' contents - but because it's done after the pg_class.relpersistence is changed they'll all not be marked as BM_PERMANENT in memory. Then the ALTER TABLE is rolled back, including the relpersistence setting. Which will unfortunately leave pages with the wrong persistency setting in memory, right? That means should I FlushRelationBuffers(rel) before change the relpersistence? Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello
Re: [HACKERS] Allow GRANT TRIGGER privilege to DROP TRIGGER (Re: Bug ##7716)
Keith Fiske ke...@omniti.com writes: I found RemoveTriggerById() in backend/commands/trigger.c which seems to be the code that actually drops the trigger. And I see in CreateTrigger() above it how to use pg_class_aclcheck() to check for valid permissions, so I was going to see about adding that code to the Remove section. However, I see no code in Remove for checking for object ownership, so I'm not sure how that is being enforced currently which would also have to be adjusted. An easy way to find the code involved in any error report is to set a breakpoint at errfinish() and then provoke the error. In this case I get #0 errfinish (dummy=0) at elog.c:410 #1 0x004f407f in aclcheck_error (aclerr=value optimized out, objectkind=ACL_KIND_CLASS, objectname=0x7f39db129d68 bobstable) at aclchk.c:3374 #2 0x004fc8e4 in check_object_ownership (roleid=207490, objtype=OBJECT_TRIGGER, address=..., objname=0x1e301e0, objargs=value optimized out, relation=0x7f39db129b50) at objectaddress.c:1160 #3 0x0055ba5d in RemoveObjects (stmt=0x1e30218) at dropcmds.c:123 #4 0x006a2540 in ExecDropStmt (stmt=0x1e30218, isTopLevel=value optimized out) at utility.c:1370 #5 0x006a2d93 in ProcessUtilitySlow (parsetree=0x1e30218, queryString=0x1e2f728 drop trigger insert_oid on bobstable;, context=PROCESS_UTILITY_TOPLEVEL, params=0x0, dest=value optimized out, completionTag=0x7fff3ad9ed90 ) at utility.c:1301 #6 0x006a370a in standard_ProcessUtility (parsetree=0x1e30218, queryString=0x1e2f728 drop trigger insert_oid on bobstable;, context=PROCESS_UTILITY_TOPLEVEL, params=0x0, dest=0x1e30670, completionTag=0x7fff3ad9ed90 ) at utility.c:793 #7 0x006a3837 in ProcessUtility (parsetree=value optimized out, queryString=value optimized out, context=value optimized out, params=value optimized out, dest=value optimized out, completionTag=value optimized out) at utility.c:311 ... A look at check_object_ownership suggests that you could take the TRIGGER case out of the generic relation path and make it a special case that allows either ownership or TRIGGER permission. TBH, though, I'm not sure this is something to pursue. We discussed all this back in 2006. As I pointed out at the time, giving somebody TRIGGER permission is tantamount to giving them full control of your account: http://www.postgresql.org/message-id/21827.1166115...@sss.pgh.pa.us because they can install a trigger that will execute arbitrary code with *your* privileges the next time you modify that table. I think we should get rid of the separate TRIGGER privilege altogether, not make it an even bigger security hole. 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] RLS Design
Brightwell, Adam adam.brightw...@crunchydatasolutions.com writes: You could do: ALTER TABLE table_name ADD POLICY policy_name (quals); ALTER TABLE table_name POLICY FOR role_name IS policy_name; ALTER TABLE table_name DROP POLICY policy_name; I am attempting to modify the grammar to support the above syntax. Unfortunately, I am encountering quite a number (280) shift/reduce errors/conflicts in bison. I have reviewed the bison documentation as well as the wiki page on resolving such conflicts. However, I am not entirely certain on the direction I should take in order to resolve these conflicts. I attempted to create a more redundant production like the wiki described, but unfortunately that was not successful. I have attached both the patch and bison report. Any help, recommendations or suggestions would be greatly appreciated. 20MB messages to the list aren't that friendly. Please don't do that again, unless asked to. FWIW, the above syntax is a nonstarter, at least unless we're willing to make POLICY a reserved word (hint: we're not). The reason is that the ADD/DROP COLUMN forms consider COLUMN to be optional, meaning that the column name could directly follow ADD; and the column type name, which could also be just a plain identifier, would directly follow that. So there's no way to resolve the ambiguity with one token of lookahead. This actually isn't just bison being stupid: in fact, you simply cannot tell whether ALTER TABLE tab ADD POLICY varchar(42); is an attempt to add a column named policy of type varchar(42), or an attempt to add a policy named varchar with quals 42. Pick a different syntax. 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] RLS Design
Adam, * Tom Lane (t...@sss.pgh.pa.us) wrote: Brightwell, Adam adam.brightw...@crunchydatasolutions.com writes: ALTER TABLE table_name ADD POLICY policy_name (quals); ALTER TABLE table_name POLICY FOR role_name IS policy_name; ALTER TABLE table_name DROP POLICY policy_name; [...] This actually isn't just bison being stupid: in fact, you simply cannot tell whether ALTER TABLE tab ADD POLICY varchar(42); is an attempt to add a column named policy of type varchar(42), or an attempt to add a policy named varchar with quals 42. Pick a different syntax. Yeah, now that we're trying to bake this into ALTER TABLE we need to be a bit more cautious. I'd think: ALTER TABLE tab POLICY ADD ... Would work though? (note: haven't looked/tested myself) Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Question about src/timezone/zic.c
* Tom Lane (t...@sss.pgh.pa.us) wrote: One thing that *would* be worth doing is merging upstream's recent updates; for all I know they've already dealt with this. According to the README, we last synced with the 2010c tzcode release, so we're short a few years worth of upstream bug fixes there. Agreed- but just to level-set expectations here, don't expect it to be a trivial or easily done thing. I looked into a bit when hitting a few Coverity complaints (which appear to be innocuous based on both my review and that of others who have commented on it) and was quite surprised at how far the code has diverged. That code appears to be quite actively updated and we may want to try and work out a way to keep up better than the occational whenever someone feels up to it and has the time to updates that we're doing now. My thought would be to try and make this part of the major version release process somehow, perhaps explicitly include it as a 'patch' or task in the last CF before we feature freeze each year. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] RLS Design
Tom, Thanks for the feedback. 20MB messages to the list aren't that friendly. Please don't do that again, unless asked to. Apologies, I didn't realize it was so large until after it was sent. At any rate, it won't happen again. FWIW, the above syntax is a nonstarter, at least unless we're willing to make POLICY a reserved word (hint: we're not). The reason is that the ADD/DROP COLUMN forms consider COLUMN to be optional, meaning that the column name could directly follow ADD; and the column type name, which could also be just a plain identifier, would directly follow that. So there's no way to resolve the ambiguity with one token of lookahead. This actually isn't just bison being stupid: in fact, you simply cannot tell whether ALTER TABLE tab ADD POLICY varchar(42); is an attempt to add a column named policy of type varchar(42), or an attempt to add a policy named varchar with quals 42. Ok. Make sense and I was afraid that was the case. Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] RLS Design
Stephen, Yeah, now that we're trying to bake this into ALTER TABLE we need to be a bit more cautious. I'd think: ALTER TABLE tab POLICY ADD ... Would work though? (note: haven't looked/tested myself) Yes, I just tested it and the following would work from a grammar perspective: ALTER TABLE table_name POLICY ADD policy_name (policy_quals) ALTER TABLE table_name POLICY DROP policy_name Though, it would obviously require the addition of POLICY to the list of unreserved keywords. I don't suspect that would be a concern, as it is not reserved, but thought I would point it out just in case. Another thought I had was, would we also want the following, so that policies could be modified? ALTER TABLE table_name POLICY ALTER policy_name (policy_quals) Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] RLS Design
Adam, * Brightwell, Adam (adam.brightw...@crunchydatasolutions.com) wrote: Yeah, now that we're trying to bake this into ALTER TABLE we need to be a bit more cautious. I'd think: ALTER TABLE tab POLICY ADD ... Would work though? (note: haven't looked/tested myself) Yes, I just tested it and the following would work from a grammar perspective: ALTER TABLE table_name POLICY ADD policy_name (policy_quals) ALTER TABLE table_name POLICY DROP policy_name Excellent, glad to hear it. Though, it would obviously require the addition of POLICY to the list of unreserved keywords. I don't suspect that would be a concern, as it is not reserved, but thought I would point it out just in case. Right, I don't anticipate anyone complaining too loudly about that.. Another thought I had was, would we also want the following, so that policies could be modified? ALTER TABLE table_name POLICY ALTER policy_name (policy_quals) Sounds like a good idea to me. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Question about src/timezone/zic.c
Stephen Frost sfr...@snowman.net writes: * Tom Lane (t...@sss.pgh.pa.us) wrote: One thing that *would* be worth doing is merging upstream's recent updates; for all I know they've already dealt with this. According to the README, we last synced with the 2010c tzcode release, so we're short a few years worth of upstream bug fixes there. Agreed- but just to level-set expectations here, don't expect it to be a trivial or easily done thing. I looked into a bit when hitting a few Coverity complaints (which appear to be innocuous based on both my review and that of others who have commented on it) and was quite surprised at how far the code has diverged. That code appears to be quite actively updated and we may want to try and work out a way to keep up better than the occational whenever someone feels up to it and has the time to updates that we're doing now. Yeah. IIRC, the existing divergences come mostly from (1) running pgindent on the zic code, which we could certainly refrain from; and (2) modifying their code to suppress assorted compiler warnings, for example they were still not using ANSI C function definition syntax last I looked. I would be loath to give up (2), so unless upstream has modernized their code it might be hard to eliminate all the divergences. If it weren't for the differing philosophies about compiler warnings, I'd wish we could create a subdirectory that was *exactly* their tzcode distribution, in much the same way that the data/ subdirectory is exactly their tzdata distribution. Then tracking their code updates would be relatively mindless. 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] gaussian distribution pgbench -- part 1/2
pgbench with gaussian exponential, part 1 of 2.
This patch is a subset of the previous patch which only adds the two
new \setrandom gaussian and exponantial variants, but not the
adapted pgbench test cases, as suggested by Fujii Masao.
There is no new code nor code changes.
The corresponding documentation has been yet again extended wrt
to the initial patch, so that what is achieved is hopefully unambiguous
(there are two mathematical formula, tasty!), in answer to Andres Freund
comments, and partly to Robert Haas comments as well.
This patch also provides several sql/pgbench scripts and a README, so
that the feature can be tested. I do not know whether these scripts
should make it to postgresql. I would say yes, otherwise there is no way
to test...
part 2 which provide adapted pgbench test cases will come later.
--
Fabien.diff --git a/contrib/pgbench/README b/contrib/pgbench/README
new file mode 100644
index 000..4b8fd59
--- /dev/null
+++ b/contrib/pgbench/README
@@ -0,0 +1,5 @@
+# gaussian and exponential tests
+# with XXX as expo or gauss
+psql test test-init.sql
+./pgbench -f test-XXX-run.sql -t 100 -P 1 test
+psql test test-XXX-check.sql
diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c
index 4aa8a50..a80c0a5 100644
--- a/contrib/pgbench/pgbench.c
+++ b/contrib/pgbench/pgbench.c
@@ -41,6 +41,7 @@
#include math.h
#include signal.h
#include sys/time.h
+#include assert.h
#ifdef HAVE_SYS_SELECT_H
#include sys/select.h
#endif
@@ -98,6 +99,8 @@ static int pthread_join(pthread_t th, void **thread_return);
#define LOG_STEP_SECONDS 5 /* seconds between log messages */
#define DEFAULT_NXACTS 10 /* default nxacts */
+#define MIN_GAUSSIAN_THRESHOLD 2.0 /* minimum threshold for gauss */
+
int nxacts = 0; /* number of transactions per client */
int duration = 0; /* duration in seconds */
@@ -471,6 +474,76 @@ getrand(TState *thread, int64 min, int64 max)
return min + (int64) ((max - min + 1) * pg_erand48(thread-random_state));
}
+/*
+ * random number generator: exponential distribution from min to max inclusive.
+ * the threshold is so that the density of probability for the last cut-off max
+ * value is exp(-exp_threshold).
+ */
+static int64
+getExponentialrand(TState *thread, int64 min, int64 max, double exp_threshold)
+{
+ double cut, uniform, rand;
+ assert(exp_threshold 0.0);
+ cut = exp(-exp_threshold);
+ /* erand in [0, 1), uniform in (0, 1] */
+ uniform = 1.0 - pg_erand48(thread-random_state);
+ /*
+ * inner expresion in (cut, 1] (if exp_threshold 0),
+ * rand in [0, 1)
+ */
+ assert((1.0 - cut) != 0.0);
+ rand = - log(cut + (1.0 - cut) * uniform) / exp_threshold;
+ /* return int64 random number within between min and max */
+ return min + (int64)((max - min + 1) * rand);
+}
+
+/* random number generator: gaussian distribution from min to max inclusive */
+static int64
+getGaussianrand(TState *thread, int64 min, int64 max, double stdev_threshold)
+{
+ double stdev;
+ double rand;
+
+ /*
+ * Get user specified random number from this loop, with
+ * -stdev_threshold stdev = stdev_threshold
+ *
+ * This loop is executed until the number is in the expected range.
+ *
+ * As the minimum threshold is 2.0, the probability of looping is low:
+ * sqrt(-2 ln(r)) = 2 = r = e^{-2} ~ 0.135, then when taking the average
+ * sinus multiplier as 2/pi, we have a 8.6% looping probability in the
+ * worst case. For a 5.0 threshold value, the looping probability
+ * is about e^{-5} * 2 / pi ~ 0.43%.
+ */
+ do
+ {
+ /*
+ * pg_erand48 generates [0,1), but for the basic version of the
+ * Box-Muller transform the two uniformly distributed random numbers
+ * are expected in (0, 1] (see http://en.wikipedia.org/wiki/Box_muller)
+ */
+ double rand1 = 1.0 - pg_erand48(thread-random_state);
+ double rand2 = 1.0 - pg_erand48(thread-random_state);
+
+ /* Box-Muller basic form transform */
+ double var_sqrt = sqrt(-2.0 * log(rand1));
+ stdev = var_sqrt * sin(2.0 * M_PI * rand2);
+
+ /*
+ * we may try with cos, but there may be a bias induced if the previous
+ * value fails the test? To be on the safe side, let us try over.
+ */
+ }
+ while (stdev -stdev_threshold || stdev = stdev_threshold);
+
+ /* stdev is in [-threshold, threshold), normalization to [0,1) */
+ rand = (stdev + stdev_threshold) / (stdev_threshold * 2.0);
+
+ /* return int64 random number within between min and max */
+ return min + (int64)((max - min + 1) * rand);
+}
+
/* call PQexec() and exit() on failure */
static void
executeStatement(PGconn *con, const char *sql)
@@ -1319,6 +1392,7 @@ top:
char *var;
int64 min,
max;
+ double threshold = 0;
char res[64];
if (*argv[2] == ':')
@@ -1364,11 +1438,11 @@ top:
}
/*
- * getrand() needs to be able to subtract max from min and add one
- * to the result without overflowing. Since we know max min, we
- * can detect overflow just by checking for a negative result. But
-
Re: [HACKERS] [BUG?] tuples from file_fdw has strange xids.
Hello, sorry, it's my bad. Kyotaro HORIGUCHI horiguchi.kyot...@lab.ntt.co.jp writes: Hello, I found that tuples come from file_fdw has strange xmin and xmax. file_fdw isn't documented to return anything useful for xmin/xmax/etc, so I don't find this surprising. The patch attached fixes the header of materialized tuple to be sane (2, 0) if the source slot was a virtual tuple in mechanism(). I don't really think it's ForeignNext's place to be doing something about this. It seems like a useless expenditure of cycles. Besides, this fails to cover e.g. postgres_fdw, which is likewise unconcerned about what it returns for system columns other than ctid. I somehow misunderstood that postgres_fdw returns (xmin, xmax) = (2, 0) but I confirmed that xmin, xmax and citd seems insane. I completely agree with you. Thank you for giving response for the stupid story. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
