Re: [HACKERS] A GUC to prevent leader processes from running subplans?
On Sun, Nov 12, 2017 at 9:18 AM, Thomas Munro wrote: > On Sat, Oct 21, 2017 at 8:09 AM, Robert Haas wrote: >> On Tue, Oct 17, 2017 at 7:27 AM, Thomas Munro >> wrote: >> >> I don't think overloading force_parallel_mode is a good idea, but >> having some other GUC for this seems OK to me. Not sure I like >> multiplex_gather, though. > > How about parallel_leader_participation = on|off? The attached > version has it that way, and adds regression tests to exercise on, off > and off-but-couldn't-start-any-workers for both kinds of gather node. > > I'm not sure why node->need_to_rescan is initialised by both > ExecGatherInit() and ExecGather(). Only the latter's value matters, > right? > I don't see anything like need_to_rescan in the GatherState node. Do you intend to say need_to_scan_locally? If yes, then I think whatever you said is right. > I've added this to the January Commitfest. > +1 to this idea. Do you think such an option at table level can be meaningful? We have a parallel_workers as a storage option for tables, so users might want leader to participate in parallelism only for some of the tables. -- 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] GatherMerge misses to push target list
On Sat, Nov 11, 2017 at 5:05 PM, Robert Haas wrote: > On Mon, Sep 18, 2017 at 7:02 AM, Rushabh Lathia > wrote: >>> In that case, can you please mark the patch [1] as ready for committer in >>> CF app >> >> Done. > > I think this patch is mostly correct, but I think the change to > planner.c isn't quite right. ordered_rel->reltarget is just a dummy > target list that produces nothing. Instead, I think we should pass > path->pathtarget, representing the idea that whatever Gather Merge > produces as output is the same as what you put into it. > Agreed. Your change looks good to me. -- 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] possible encoding issues with libxml2 functions
2017-11-11 21:19 GMT+01:00 Noah Misch : > On Sun, Nov 05, 2017 at 06:10:04PM +0100, Pavel Stehule wrote: > > Hi > > > > 2017-11-05 4:07 GMT+01:00 Noah Misch : > > > > > On Tue, Oct 17, 2017 at 06:06:40AM +0200, Pavel Stehule wrote: > > > > Please, if you can, try it write. I am little bit lost :) > > > > > > I'm attaching the patch I desired. Please review. This will probably > miss > > > this week's minor releases. If there's significant support, I could > > > instead > > > push before the wrap. > > > > > > > I have not any objection to this solution. It fixes my regress tests too. > > > > I checked it and it is working. > > Pushed, but the buildfarm shows I didn't get the test quite right for the > non-xml, non-UTF8 case. Fixing. > Thank you Pavel
Re: [HACKERS] A GUC to prevent leader processes from running subplans?
On Sat, Oct 21, 2017 at 8:09 AM, Robert Haas wrote: > On Tue, Oct 17, 2017 at 7:27 AM, Thomas Munro > wrote: >> While testing parallelism work I've wanted to be able to prevent >> gather nodes from running the plan in the leader process, and I've >> heard others say the same. One way would be to add a GUC >> "multiplex_gather", like in the attached patch. If you set it to off, >> Gather and Gather Merge won't run the subplan unless they have to >> because no workers could be launched. I thought about adding a new >> value for force_parallel_mode instead, but someone mentioned they >> might want to do this on a production system too and >> force_parallel_mode is not really for end users. Better ideas? > > I don't think overloading force_parallel_mode is a good idea, but > having some other GUC for this seems OK to me. Not sure I like > multiplex_gather, though. How about parallel_leader_participation = on|off? The attached version has it that way, and adds regression tests to exercise on, off and off-but-couldn't-start-any-workers for both kinds of gather node. I'm not sure why node->need_to_rescan is initialised by both ExecGatherInit() and ExecGather(). Only the latter's value matters, right? I've added this to the January Commitfest. -- Thomas Munro http://www.enterprisedb.com parallel-leader-participation-v1.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] pgbench regression test failure
The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: not tested
Spec compliant: not tested
Documentation:not tested
This causes the pgbench tests to fail (consistently) with
not ok 194 - pgbench late throttling stdout /(?^:processed: [01]/10)/
When I run pgbench manually I get (-t 10 --rate=10 --latency-limit=1 -n -r)
number of transactions actually processed: 10/10
number of transactions skipped: 10 (100.000 %)
Prior to the patch I was getting.
number of transactions actually processed: 0/10
number of transactions skipped: 10 (100.000 %)
@@ -3539,7 +3542,7 @@ printResults(TState *threads, StatsData *total,
instr_time total_time,^M
{^M
printf("number of transactions per client: %d\n", nxacts);^M
printf("number of transactions actually processed: "
INT64_FORMAT "/%d\n",^M
- total->cnt - total->skipped, nxacts * nclients);^M
+ total->cnt, nxacts * nclients);^M
I think you want ntx instead of total->cnt here.
The new status of this patch is: Waiting on Author
--
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] Simplify ACL handling for large objects and removal of superuser() checks
> On Nov 10, 2017, at 3:58 PM, Stephen Frost wrote: > > Michael, Tom, > > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> On Fri, Nov 10, 2017 at 10:00 AM, Tom Lane wrote: >>> Stephen Frost writes: I'm guessing no, which essentially means that *we* consider access to lo_import/lo_export to be equivilant to superuser and therefore we're not going to implement anything to try and prevent the user who has access to those functions from becoming superuser. If we aren't willing to do that, then how can we really say that there's some difference between access to these functions and being a superuser? >>> >>> We seem to be talking past each other. Yes, if a user has malicious >>> intentions, it's possibly to parlay lo_export into obtaining a superuser >>> login (I'm less sure that that's necessarily true for lo_import). >>> That does NOT make it "equivalent", except perhaps in the view of someone >>> who is only considering blocking malevolent actors. It does not mean that >>> there's no value in preventing a task that needs to run lo_export from >>> being able to accidentally destroy any data in the database. There's a >>> range of situations where you are concerned about accidents and errors, >>> not malicious intent; but your argument ignores those use-cases. >> >> That will not sound much as a surprise as I spawned the original >> thread, but like Robert I understand that getting rid of all superuser >> checks is a goal that we are trying to reach to allow admins to have >> more flexibility in handling permissions to a subset of objects. >> Forcing an admin to give full superuser rights to one user willing to >> work only on LOs import and export is a wrong concept. > > The problem I have with this is that 'full superuser rights' actually > are being granted by this. You're able to read any file and write any > file on the filesystem that the PG OS user can. How is that not 'full > superuser rights'? The concerns about a mistake being made are just as > serious as they are with someone who has superuser rights as someone > could pretty easily end up overwriting the control file or various other > extremely important bits of the system. This isn't just about what > 'blackhats' can do with this. > > As I mentioned up-thread, if we want to make it so that non-superusers > can use lo_import/lo_export, then we should do that in a way that > allows the administrator to specify exactly the rights they really want > to give, based on a use-case for them. There's no use-case for allowing > someone to use lo_export() to overwrite pg_control. The use-case would > be to allow them to export to a specific directory (or perhaps a set of > directories), or to read from same. The same is true of COPY. Further, > I wonder what would happen if we allow this and then someone comes along > and re-proposes the 'CREATE DIRECTORY' concept that I had once proposed > (ideally with things cleaned up and tightened up to avoid there being > issues using it) to address the actual use-case for these functions to > be available to a non-superuser. We wouldn't be able to change these > functions to start checking the 'directory' rights or we would break > existing non-superuser usage of them. I imagine we could create > additional functions which check the 'directory' privileges, but that's > hardly ideal, imv. > > I'm disappointed to apparently be in the minority on this, but that > seems to be the case unless someone else wishes to comment. While I > appreciate the discussion, I'm certainly no more convinced today than I > was when I first saw this go in that allowing these functions to be > granted to non-superusers does anything but effectively make them into > superusers who aren't explicitly identified as superusers. Just to help understand your concern, and not to propose actual features, would it help if there were (1) a function, perhaps pg_catalog.pg_exploiters(), which would return all roles that have been granted exploitable privileges, such that it would be easier to identify all superusers, including those whose superuserishness derives from a known export, and (2) a syntax change for GRANT that would require an extra token, so that you'd have to write something like GRANT EXPLOITABLE lo_export TO trusted_user_foo so that you couldn't unknowingly grant a dangerous privilege. Or is there more to it than that? mark -- 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 #14897: Segfault on statitics SQL request
Hi, Just to show comment from: gcc/gcc/testsuite/gcc.target/x86_64/abi/defines.h /* These defines control the building of the list of types to check. There is a string identifying the type (with a comma after), a size of the type (also with a comma and an integer for adding to the total amount of types) and an alignment of the type (which is currently not really needed since the abi specifies that alignof == sizeof for all scalar types). */ Specifically "the abi specifies that alignof == sizeof for all scalar types" Vladimir Kokovic Belgrade, 11.November 2017 -- 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] Partition-wise aggregation/grouping
On 10/27/2017 02:01 PM, Jeevan Chalke wrote: Hi, Attached new patch-set here. Changes include: 1. Added separate patch for costing Append node as discussed up-front in the patch-set. 2. Since we now cost Append node, we don't need partition_wise_agg_cost_factor GUC. So removed that. The remaining patch hence merged into main implementation patch. 3. Updated rows in test-cases so that we will get partition-wise plans. Thanks I applied partition-wise-agg-v6.tar.gz patch to the master and use shard.sh example from https://www.postgresql.org/message-id/14577.1509723225%40localhost Plan for count(*) is the following: shard=# explain select count(*) from orders; QUERY PLAN --- Finalize Aggregate (cost=100415.29..100415.30 rows=1 width=8) -> Append (cost=50207.63..100415.29 rows=2 width=8) -> Partial Aggregate (cost=50207.63..50207.64 rows=1 width=8) -> Foreign Scan on orders_0 (cost=101.00..50195.13 rows=5000 width=0) -> Partial Aggregate (cost=50207.63..50207.64 rows=1 width=8) -> Foreign Scan on orders_1 (cost=101.00..50195.13 rows=5000 width=0) We really calculate partial aggregate for each partition, but to do we still have to fetch all data from remote host. So for foreign partitions such plans is absolutely inefficient. Amy be it should be combined with some other patch? For example, with agg_pushdown_v4.tgz patch https://www.postgresql.org/message-id/14577.1509723225%40localhost ? But it is not applied after partition-wise-agg-v6.tar.gz patch. Also postgres_fdw in 11dev is able to push down aggregates without agg_pushdown_v4.tgz patch. In 0009-Teach-postgres_fdw-to-push-aggregates-for-child-rela.patch there is the following check: /* Partial aggregates are not supported. */ + if (extra->isPartial) + return; If we just comment this line then produced plan will be the following: shard=# explain select sum(product_id) from orders; QUERY PLAN Finalize Aggregate (cost=308.41..308.42 rows=1 width=8) -> Append (cost=144.18..308.41 rows=2 width=8) -> Foreign Scan (cost=144.18..154.20 rows=1 width=8) Relations: Aggregate on (public.orders_0 orders) -> Foreign Scan (cost=144.18..154.20 rows=1 width=8) Relations: Aggregate on (public.orders_1 orders) (6 rows) And it is actually desired plan! Obviously such approach will not always work. FDW really doesn't support partial aggregates now. But for most frequently used aggregates: sum, min, max, count aggtype==aggtranstype and there is no difference between partial and normal aggregate calculation. So instead of (extra->isPartial) condition we can add more complex check which will traverse pathtarget expressions and check if it can be evaluated in this way. Or... extend FDW API to support partial aggregation. But even the last plan is not ideal: it will calculate predicates at each remote node sequentially. There is parallel append patch: https://www.postgresql.org/message-id/CAJ3gD9ctEcrVUmpY6fq_JUB6WDKGXAGd70EY68jVFA4kxMbKeQ%40mail.gmail.com but ... FDW doesn't support parallel scan, so parallel append can not be applied in this case. And we actually do not need parallel append with all its dynamic workers here. We just need to start commands at all remote servers and only after it fetch results (which can be done sequentially). I am investigating problem of efficient execution of OLAP queries on sharded tables (tables with remote partitions). After reading all this threads and corresponding patches, it seems to me that we already have most of parts of the puzzle, what we need is to put them on right places and may be add missed ones. I wonder if somebody is busy with it and can I somehow help here? Also I am not quite sure about the best approach with parallel execution of distributed query at all nodes. Should we make postgres_fdw parallel safe and use parallel append? How difficult it will be? Or in addition to parallel append we should also have "asynchronous append" which will be able to initiate execution at all nodes? It seems to be close to merge append, because it should simultaneously traverse all cursors. Looks like second approach is easier for implementation. But in case of sharded table, distributed query may need to traverse both remote and local shards and this approach doesn't allow to processed several local shards in parallel. -- Konstantin Knizhnik Postgres Professional: http://www.postgrespro.com The Russian Postgres 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] possible encoding issues with libxml2 functions
On Sun, Nov 05, 2017 at 06:10:04PM +0100, Pavel Stehule wrote: > Hi > > 2017-11-05 4:07 GMT+01:00 Noah Misch : > > > On Tue, Oct 17, 2017 at 06:06:40AM +0200, Pavel Stehule wrote: > > > Please, if you can, try it write. I am little bit lost :) > > > > I'm attaching the patch I desired. Please review. This will probably miss > > this week's minor releases. If there's significant support, I could > > instead > > push before the wrap. > > > > I have not any objection to this solution. It fixes my regress tests too. > > I checked it and it is working. Pushed, but the buildfarm shows I didn't get the test quite right for the non-xml, non-UTF8 case. Fixing. -- 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] Simplify ACL handling for large objects and removal of superuser() checks
Stephen Frost writes: > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> Forcing an admin to give full superuser rights to one user willing to >> work only on LOs import and export is a wrong concept. > The problem I have with this is that 'full superuser rights' actually > are being granted by this. You're able to read any file and write any > file on the filesystem that the PG OS user can. How is that not 'full > superuser rights'? It doesn't cause, say, "DELETE FROM pg_proc;" to succeed. You're still not getting the point that this is for people who want self-imposed restrictions. Sure, you can't give out lo_export privilege to someone you would not trust with superuser. But somebody who needs lo_export, and is trustworthy enough to have it, may still wish to do the inside-the-database part of their work with less than full superuser, just as a safety measure. It's the *exact same* reason why cautious people use "sudo" rather than just running as root all the time. > As I mentioned up-thread, if we want to make it so that non-superusers > can use lo_import/lo_export, then we should do that in a way that > allows the administrator to specify exactly the rights they really want > to give, based on a use-case for them. Our current answer for that is wrapper functions. This patch does not make that answer any less applicable than before. > I wonder what would happen if we allow this and then someone comes along > and re-proposes the 'CREATE DIRECTORY' concept that I had once proposed > (ideally with things cleaned up and tightened up to avoid there being > issues using it) to address the actual use-case for these functions to > be available to a non-superuser. We wouldn't be able to change these > functions to start checking the 'directory' rights or we would break > existing non-superuser usage of them. This is a straw man argument --- if we tighten up the function to check this as-yet-nonexistent feature, how is that not breaking existing use-cases anyway? 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] [PATCH] A hook for session start
On Sat, Nov 11, 2017 at 6:48 AM, Michael Paquier wrote: > > On Sat, Nov 11, 2017 at 12:49 AM, Fabrízio de Royes Mello > wrote: > > New version attached. > > Thanks. > > +++ b/src/test/modules/Makefile > test_extensions \ > + test_session_hooks \ > test_parser > Better if that's in alphabetical order. > Fixed. > That's a nit though, so I am switching the patch as ready for committer. > Thank you so much for your review. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL >> Timbira: http://www.timbira.com.br >> Blog: http://fabriziomello.github.io >> Linkedin: http://br.linkedin.com/in/fabriziomello >> Twitter: http://twitter.com/fabriziomello >> Github: http://github.com/fabriziomello diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 05c5c19..d3156ad 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -169,6 +169,9 @@ static ProcSignalReason RecoveryConflictReason; static MemoryContext row_description_context = NULL; static StringInfoData row_description_buf; +/* Hook for plugins to get control at start of session */ +session_start_hook_type session_start_hook = NULL; + /* * decls for routines only used in this file * @@ -3857,6 +3860,9 @@ PostgresMain(int argc, char *argv[], if (!IsUnderPostmaster) PgStartTime = GetCurrentTimestamp(); + if (session_start_hook) + (*session_start_hook) (); + /* * POSTGRES main processing loop begins here * diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 20f1d27..16ec376 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -76,6 +76,8 @@ static bool ThereIsAtLeastOneRole(void); static void process_startup_options(Port *port, bool am_superuser); static void process_settings(Oid databaseid, Oid roleid); +/* Hook for plugins to get control at end of session */ +session_end_hook_type session_end_hook = NULL; /*** InitPostgres support ***/ @@ -1154,6 +1156,10 @@ ShutdownPostgres(int code, Datum arg) * them explicitly. */ LockReleaseAll(USER_LOCKMETHOD, true); + + /* Hook at session end */ + if (session_end_hook) + (*session_end_hook) (); } diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h index f8c535c..9f05bfb 100644 --- a/src/include/tcop/tcopprot.h +++ b/src/include/tcop/tcopprot.h @@ -35,6 +35,13 @@ extern PGDLLIMPORT const char *debug_query_string; extern int max_stack_depth; extern int PostAuthDelay; +/* Hook for plugins to get control at start and end of session */ +typedef void (*session_start_hook_type) (void); +typedef void (*session_end_hook_type) (void); + +extern PGDLLIMPORT session_start_hook_type session_start_hook; +extern PGDLLIMPORT session_end_hook_type session_end_hook; + /* GUC-configurable parameters */ typedef enum diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index b7ed0af..7246552 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -15,6 +15,7 @@ SUBDIRS = \ test_pg_dump \ test_rbtree \ test_rls_hooks \ + test_session_hooks \ test_shm_mq \ worker_spi diff --git a/src/test/modules/test_session_hooks/.gitignore b/src/test/modules/test_session_hooks/.gitignore new file mode 100644 index 000..5dcb3ff --- /dev/null +++ b/src/test/modules/test_session_hooks/.gitignore @@ -0,0 +1,4 @@ +# Generated subdirectories +/log/ +/results/ +/tmp_check/ diff --git a/src/test/modules/test_session_hooks/Makefile b/src/test/modules/test_session_hooks/Makefile new file mode 100644 index 000..c5c3860 --- /dev/null +++ b/src/test/modules/test_session_hooks/Makefile @@ -0,0 +1,21 @@ +# src/test/modules/test_session_hooks/Makefile + +MODULES = test_session_hooks +PGFILEDESC = "test_session_hooks - Test session hooks with an extension" + +EXTENSION = test_session_hooks +DATA = test_session_hooks--1.0.sql + +REGRESS = test_session_hooks +REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/test_session_hooks/session_hooks.conf + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/test_session_hooks +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git a/src/test/modules/test_session_hooks/README b/src/test/modules/test_session_hooks/README new file mode 100644 index 000..9cb4202 --- /dev/null +++ b/src/test/modules/test_session_hooks/README @@ -0,0 +1,2 @@ +test_session_hooks is an example of how to use session start and end +hooks. diff --git a/src/test/modules/test_session_hooks/expected/test_session_hooks.out b/src/test/modules/test_session_hooks/expected/test_session_hooks.out new file mode 100644 index 000..be1b949 --- /dev/null +++ b/src/tes
Re: [HACKERS] parallelize queries containing initplans
On Sat, Nov 11, 2017 at 12:15 AM, Robert Haas wrote:
> On Tue, Nov 7, 2017 at 4:45 AM, Amit Kapila wrote:
>> As mentioned, changed the status of the patch in CF app.
>
> I spent some time reviewing this patch today and found myself still
> quite uncomfortable with the fact that it was adding execution-time
> work to track the types of parameters - types that would usually not
> even be used. I found the changes to nodeNestLoop.c to be
> particularly objectionable, because we could end up doing the work
> over and over when it is actually not needed at all, or at most once.
>
That's right, but we are just accessing tuple descriptor to get the
type, there shouldn't be much work involved in that. However, I think
your approach has a merit that we don't need to even do that during
execution time.
> I decided to try instead teaching the planner to keep track of the
> types of PARAM_EXEC parameters as they were created, and that seems to
> work fine. See 0001, attached.
>
This looks good to me.
> 0002, attached, is my worked-over version of the rest of the patch. I
> moved the code that serializes and deserializes PARAM_EXEC from
> nodeSubplan.c -- which seemed like a strange choice - to
> execParallel.c.
>
I have tried to follow the practice we have used for param extern
params (SerializeParamList is in params.c) and most of the handling of
initplans is done in nodeSubplan.c, so I choose to keep the newly
added functions there. However, I think keeping it in execParallel.c
is also okay as we do it for serialize plan.
> I removed the type OID from the serialization format
> because there's no reason to do that any more; the worker already
> knows the types from the plan. I did some renaming of the functions
> involved and some adjustment of the comments to refer to "PARAM_EXEC
> parameters" instead of initPlan parameters, because there's no reason
> that I can see why this can only work for initPlans. A Gather node on
> the inner side of a nested loop doesn't sound like a great idea, but I
> think this infrastructure could handle it (though it would need some
> more planner work).
>
I think it would need some work in execution as well because the size
won't be fixed in that case for varchar type of params. We might end
up with something different as well.
> I broke a lot of long lines in your version of
> the patch into multiple lines; please try to be attentive to this
> issue when writing patches in general, as it is a bit tedious to go
> through and insert line breaks in many places.
>
Okay, but I sometimes rely on pgindent for such things as for few
things it becomes difficult to decide which way it will be better.
> Please let me know your thoughts on the attached patches.
>
Few minor comments:
1.
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -23,6 +23,7 @@
#include "postgres.h"
+#include "executor/execExpr.h"
#include "executor/execParallel.h"
#include "executor/executor.h"
#include "executor/nodeBitmapHeapscan.h"
@@ -31,6 +32,7 @@
#include "executor/nodeIndexscan.h"
#include "executor/nodeIndexonlyscan.h"
#include "executor/nodeSeqscan.h"
+#include "executor/nodeSubplan.h"
This is not required if we move serialize and other functions to execParallel.c
2.
+set_param_references(PlannerInfo *root, Plan *plan)
+{
+ Assert(IsA(plan, Gather) ||IsA(plan, GatherMerge));
I think there should be a space after || operator.
3.
+/*
+ * Serialize ParamExecData params corresponding to initplans.
+ *
..
+/*
+ * Restore ParamExecData params corresponding to initplans.
+ */
Shouldn't we change the reference to initplans here as well?
I have fixed the first two in attached patch and left the last one as
I was not sure what you have in mind
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
0002-pq-pushdown-initplan-rebased-1.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PROPOSAL] Temporal query processing with range types
On Fri, Oct 6, 2017 at 3:22 PM, Mike Rylander wrote:
> I've also been following this feature with great interest, and would
> definitely throw whatever tiny weight I have, sitting out here in the
> the peanut gallery, behind accepting the ALIGN and NORMALIZE syntax.
> I estimate that about a third of the non-trivial queries in the
> primary project I work on (and have, on Postgres, for the last 13+
> years) would be simpler with support of the proposed syntax, and some
> of the most complex business logic would be simplified nearly to the
> point of triviality.
This is really good input. If the feature weren't useful, then it
wouldn't make sense to try to figure out how to integrate it, but if
it is, then we should try.
I don't think that implementing a feature like this by SQL
transformation can work. It's certainly got the advantage of
simplicity of implemention, but there are quite a few things that seem
like they won't always work correctly. For instance:
+ * INPUT:
+ * (r ALIGN s ON q WITH (r.t, s.t)) c
+ *
+ * where r and s are input relations, q can be any
+ * join qualifier, and r.t, s.t can be any column name. The latter
+ * represent the valid time intervals, that is time point start,
+ * and time point end of each tuple for each input relation. These
+ * are two half-open, i.e., [), range typed values.
+ *
+ * OUTPUT:
+ * (
+ * SELECT r.*, GREATEST(LOWER(r.t), LOWER(s.t)) P1,
+ * LEAST(UPPER(r.t), UPPER(s.t)) P2
+ * FROM
+ * (
+ * SELECT *, row_id() OVER () rn FROM r
+ * ) r
+ * LEFT OUTER JOIN
+ * s
+ * ON q AND r.t && s.t
+ * ORDER BY rn, P1, P2
+ * ) c
One problem with this is that we end up looking up functions in
pg_catalog by name: LOWER, UPPER, LEAST, GREATEST. In particular,
when we do this...
+fcUpperRarg = makeFuncCall(SystemFuncName("upper"),
+ list_make1(crRargTs),
+ UNKNOWN_LOCATION);
...we're hoping and praying that we're going to latch onto the first of these:
rhaas=# \df upper
List of functions
Schema | Name | Result data type | Argument data types | Type
+---+--+-+
pg_catalog | upper | anyelement | anyrange| normal
pg_catalog | upper | text | text| normal
(2 rows)
But that's only true as long as there isn't another function in
pg_catalog with a match to the specific range type that is being used
here, and there's nothing to stop a user from creating one, and then
their query, which does not anywhere in its query text mention the
name of that function, will start failing. We're not going to accept
that limitation. Looking up functions by name rather than by OID or
using an opclass or something is pretty much a death sentence for a
core feature, and this patch does a lot of it.
A related problem is that, because all of this transformation is being
done in the parser, when you use this temporal syntax to create a
view, and then run pg_dump to dump that view, you are going to (I
think) get the transformed version, not the original. Things like
transformTemporalClause are going to have user-visible effects: the
renaming you do there will (I think) be reflected in the deparsed
output of views. That's not good. Users have a right to expect that
what comes out of deparsing will at least resemble what they put in.
Error reporting might be a problem too: makeTemporalQuerySkeleton is
creating parse nodes that have no location, so if an error develops at
that point, how will the user correlate that with what they typed in?
I suspect there are also problems with plan invalidation. Any
decisions we make at parse time are fixed forever. DDL changes can
force a re-plan, but not a re-parse.
Overall, I think that the whole approach here probably needs to be
scrapped and rethought. The stuff this patch is doing really belongs
in the optimizer, not the parser, I think. It could possibly happen
at a relatively early stage in the optimizer so that the rest of the
optimizer can see the results of the transformation and, well,
optimize. But parse time is way too early.
Unrelated to the above, this patch introduces various kinds of helper
functions which are general-purpose in function but dumped in with the
temporal support because it happens to use them. For instance:
+static Form_pg_type
+typeGet(Oid id)
+{
+HeapTupletp;
+Form_pg_type typtup;
+
+tp = SearchSysCache1(TYPEOID, ObjectIdGetDatum(id));
+if (!HeapTupleIsValid(tp))
+ereport(ERROR,
+(errcode(ERROR),
+ errmsg("cache lookup failed for type %u", id)));
+
+typtup = (Form_pg_type) GETSTRUCT(tp);
+ReleaseSysCache(tp);
+return typtup;
+}
A function as general as typeGet() certainly does not belong in
parse_clause.c in the middle of a
Re: [HACKERS] GatherMerge misses to push target list
On Mon, Sep 18, 2017 at 7:02 AM, Rushabh Lathia wrote: >> In that case, can you please mark the patch [1] as ready for committer in >> CF app > > Done. I think this patch is mostly correct, but I think the change to planner.c isn't quite right. ordered_rel->reltarget is just a dummy target list that produces nothing. Instead, I think we should pass path->pathtarget, representing the idea that whatever Gather Merge produces as output is the same as what you put into it. See attached. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company pushdown-gathermerge-tlist.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] Log SSL certificate verification errors
On 11 Nov 2017, at 6:23 AM, Michael Paquier wrote: >> Currently neither the server side nor the client side SSL certificate verify >> callback does anything, leading to potential hair-tearing-out moments. >> >> The following patch to master implements logging of all certificate >> verification failures, as well as (crucially) which certificates failed to >> verify, and at what depth, so the admin can zoom in straight onto the >> problem without any guessing. > > Could you attach as a file to this thread a patch that can be easily > applied? Using git --format-patch or simply diff is just fine. I’ve attached it as a separate attachment. The default behaviour of patch is to ignore all lines before and after the patch, so you can use my entire email as an input to patch and it will work (This is what git format-patch does, create something that looks like an email). > Here are also some community guidelines on the matter: > https://wiki.postgresql.org/wiki/Submitting_a_Patch > > And if you are looking for feedback, you should register it to the > next commit fest: > https://commitfest.postgresql.org/16/ I shall do! Regards, Graham — postgresql-log-cert-verification.diff Description: Binary data smime.p7s Description: S/MIME cryptographic signature
Re: [HACKERS] [PATCH] A hook for session start
On Sat, Nov 11, 2017 at 12:49 AM, Fabrízio de Royes Mello wrote: > New version attached. Thanks. +++ b/src/test/modules/Makefile test_extensions \ + test_session_hooks \ test_parser Better if that's in alphabetical order. That's a nit though, so I am switching the patch as ready for committer. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
