Re: [HACKERS] Prepared statements and generic plans
Bruce Momjian wrote: > OK, updated version attached. I added "potential" to the first > paragraph, and added "estimated cost" to the later part, fixed the > "cheaper than", and clarified that we add the plan time cost to the > non-generic plan, which is how it can be cheaper than the generic plan. > I also moved the "Once a generic plan is chosen" line. > > Yeah, that's a lot of changes, but they all improved the text. Thanks. Thanks for working on this. !Prepared statements can optionally use generic plans rather than !re-planning with each set of supplied EXECUTE values. !This occurs immediately for prepared statements with no parameters; !otherwise it occurs only after five or more executions produce estimated !plan costs, with planning overhead added, that are, on average, more !expensive than the generic plan cost. The following might be easier to understand: ... after five or more executions produce plans whose estimated cost average (including planning overhead) is more expensive than the generic plan cost estimate. !A generic plan assumes each value supplied to EXECUTE ... assumes *that* each value ... !is one of the column's distinct values and that column values are !uniformly distributed. For example, if statistics records three Shouldn't it be "record"? The documentation treats "statistics" as a plural word throughout. !distinct column values, a generic plan assumes a column equality !comparison will match 33% of processed rows. Column statistics ... assumes *that* a column equality comparison will match 33% of *the* processed rows. !also allows generic plans to accurately compute the selectivity of Column statistics also *allow* ... !unique columns. Comparisons on non-uniformly-distributed columns and !specification of non-existent values affects the average plan cost, !and hence if and when a generic plan is chosen. (Disclaimer: I am not a native speaker.) Other than that, a hearty +1. Yours, Laurenz Albe -- 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] [sqlsmith] Failed assertion in joinrels.c
On Sun, Jun 5, 2016 at 11:16 PM, Tom Lane wrote:
> Michael Paquier writes:
>> Still, on back branches I think that it would be a good idea to have a
>> better error message for the ones that already throw an error, like
>> "trigger with OID %u does not exist". Switching from elog to ereport
>> would be a good idea, but wouldn't it be a problem to change what is
>> user-visible?
>
> If we're going to touch this behavior in the back branches, I would
> vote for changing it the same as we do in HEAD. I do not think that
> making a user-visible behavior change in a minor release and then a
> different change in the next major is good strategy.
So, I have finished with the patch attached that changes all the
*def() functions to return NULL when an object does not exist. Some
callers of the index and constraint definitions still expect a cache
lookup error if the object does not exist, and this patch is careful
about that. I think that it would be nice to get that in 9.6, but I
won't fight hard for it either. So I am parking it for now in the
upcoming CF.
> But, given the relative shortage of complaints from the field, I'd
> be more inclined to just do nothing in back branches. There might
> be people out there with code depending on the current behavior,
> and they'd be annoyed if we change it in a minor release.
Sure. That's the safest position. Thinking about the existing behavior
for some of the index and constraint callers, even just changing the
error message does not bring much as some of them really expect to
fail with a cache lookup error.
--
Michael
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 8cb3075..0651cb2 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -314,9 +314,9 @@ static char *pg_get_ruledef_worker(Oid ruleoid, int prettyFlags);
static char *pg_get_indexdef_worker(Oid indexrelid, int colno,
const Oid *excludeOps,
bool attrsOnly, bool showTblSpc,
- int prettyFlags);
+ int prettyFlags, bool missing_ok);
static char *pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
- int prettyFlags);
+ int prettyFlags, bool missing_ok);
static text *pg_get_expr_worker(text *expr, Oid relid, const char *relname,
int prettyFlags);
static int print_function_arguments(StringInfo buf, HeapTuple proctup,
@@ -466,9 +466,16 @@ pg_get_ruledef(PG_FUNCTION_ARGS)
{
Oid ruleoid = PG_GETARG_OID(0);
int prettyFlags;
+ char *res;
prettyFlags = PRETTYFLAG_INDENT;
- PG_RETURN_TEXT_P(string_to_text(pg_get_ruledef_worker(ruleoid, prettyFlags)));
+
+ res = pg_get_ruledef_worker(ruleoid, prettyFlags);
+
+ if (res == NULL)
+ PG_RETURN_NULL();
+
+ PG_RETURN_TEXT_P(string_to_text(res));
}
@@ -478,9 +485,16 @@ pg_get_ruledef_ext(PG_FUNCTION_ARGS)
Oid ruleoid = PG_GETARG_OID(0);
bool pretty = PG_GETARG_BOOL(1);
int prettyFlags;
+ char *res;
prettyFlags = pretty ? PRETTYFLAG_PAREN | PRETTYFLAG_INDENT : PRETTYFLAG_INDENT;
- PG_RETURN_TEXT_P(string_to_text(pg_get_ruledef_worker(ruleoid, prettyFlags)));
+
+ res = pg_get_ruledef_worker(ruleoid, prettyFlags);
+
+ if (res == NULL)
+ PG_RETURN_NULL();
+
+ PG_RETURN_TEXT_P(string_to_text(res));
}
@@ -532,7 +546,12 @@ pg_get_ruledef_worker(Oid ruleoid, int prettyFlags)
if (spirc != SPI_OK_SELECT)
elog(ERROR, "failed to get pg_rewrite tuple for rule %u", ruleoid);
if (SPI_processed != 1)
- appendStringInfoChar(&buf, '-');
+ {
+ /*
+ * There is no tuple data available here, just keep the output buffer
+ * empty.
+ */
+ }
else
{
/*
@@ -549,6 +568,9 @@ pg_get_ruledef_worker(Oid ruleoid, int prettyFlags)
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
+ if (buf.len == 0)
+ return NULL;
+
return buf.data;
}
@@ -564,9 +586,16 @@ pg_get_viewdef(PG_FUNCTION_ARGS)
/* By OID */
Oid viewoid = PG_GETARG_OID(0);
int prettyFlags;
+ char *res;
prettyFlags = PRETTYFLAG_INDENT;
- PG_RETURN_TEXT_P(string_to_text(pg_get_viewdef_worker(viewoid, prettyFlags, WRAP_COLUMN_DEFAULT)));
+
+ res = pg_get_viewdef_worker(viewoid, prettyFlags, WRAP_COLUMN_DEFAULT);
+
+ if (res == NULL)
+ PG_RETURN_NULL();
+
+ PG_RETURN_TEXT_P(string_to_text(res));
}
@@ -577,9 +606,16 @@ pg_get_viewdef_ext(PG_FUNCTION_ARGS)
Oid viewoid = PG_GETARG_OID(0);
bool pretty = PG_GETARG_BOOL(1);
int prettyFlags;
+ char *res;
prettyFlags = pretty ? PRETTYFLAG_PAREN | PRETTYFLAG_INDENT : PRETTYFLAG_INDENT;
- PG_RETURN_TEXT_P(string_to_text(pg_get_viewdef_worker(viewoid, prettyFlags, WRAP_COLUMN_DEFAULT)));
+
+ res = pg_get_viewdef_worker(viewoid, prettyFlags, WRAP_COLUMN_DEFAULT);
+
+ if (res == NULL)
+ PG_RETURN_NULL();
+
+ PG_RETURN_TEXT_P(string_to_text(res));
}
Datum
@@ -589,10 +625,17 @@ pg_get_viewdef_wrap(PG_FUNCTION_ARGS)
Oid viewoid = PG_GETARG_OID(0);
int wrap = PG_GETARG_INT32(1);
int prettyFlags;
+ char *res;
/* calli
[HACKERS] installcheck failing on psql_crosstab
Hi all, With a low value of work_mem, like 1MB, I am noticing that the new psql_crosstab is generating a couple of diffs with installcheck (tested only on OSX 10.11): *** *** 127,134 \crosstabview v h i v | h0 | h1 | h2 | h4 | #null# +++++ ! v1 | #null# || 3 +|| ! ||| 7 || v2 || 3 ||| v0 |||| 4 +| 5 |||| -3 | --- 127,134 \crosstabview v h i v | h0 | h1 | h2 | h4 | #null# +++++ ! v1 | #null# || 7 +|| ! ||| 3 || v2 || 3 ||| v0 |||| 4 +| 5 |||| -3 | *** *** 143,150 +--+-+- h0 | baz | | h1 | | bar | ! h2 | foo +| | ! | quux | | h4 | | | qux+ | | | dbl | | | qux --- 143,150 +--+-+- h0 | baz | | h1 | | bar | ! h2 | quux+| | ! | foo | | h4 | | | qux+ | | | dbl | | | qux *** *** 156,163 \crosstabview 1 "h" 4 v | h0 | h1 | h2 | h4 | +-+-+--+-+- ! v1 | baz | | foo +| | ! | | | quux | | v2 | | bar | | | v0 | | | | qux+| qux | | | | dbl | --- 156,163 \crosstabview 1 "h" 4 v | h0 | h1 | h2 | h4 | +-+-+--+-+- ! v1 | baz | | quux+| | ! | | | foo | | v2 | | bar | | | v0 | | | | qux+| qux | | | | dbl | == I know that we guarantee that make installcheck may not work on many target servers as a lot of tests are very GUC-sensitive, but this looks a bit oversensitive to me, especially knowing that it is the only diff generated by the whole test suite. Don't you think that those tests could be made more portable? 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] Reviewing freeze map code
On Sat, Jun 4, 2016 at 1:46 PM, Masahiko Sawada wrote:
> On Sat, Jun 4, 2016 at 12:59 AM, Robert Haas wrote:
>> On Fri, Jun 3, 2016 at 11:21 AM, Masahiko Sawada
>> wrote:
Can you submit that part as a separate patch?
>>>
>>> Attached.
>>
>> Thanks, committed.
>>
> I'm address the review comment of 7087166 commit, and will post the patch.
When?
>>>
>>> On Saturday.
>>
>> Great. Will that address everything for this open item, then?
>>
>
> Attached patch for commit 7087166 on another mail.
> I think that only the test tool for visibility map is remaining and
> under the discussion.
> Even if we have verification tool or function for visibility map, we
> cannot repair the contents of visibility map if we turned out that
> contents of visibility map is something wrong.
> So I think we should have the way that re-generates the visibility map.
> For this purpose, doing vacuum while ignoring visibility map by a new
> option or new function is one idea.
> But IMHO, it's not good idea to allow a function to do vacuum, and
> expanding the VACUUM syntax might be somewhat overkill.
>
> So other idea is to have GUC parameter, vacuum_even_frozen_page for example.
> If this parameter is set true (false by default), we do vacuum whole
> table forcibly and re-generate visibility map.
> The advantage of this idea is that we don't necessary to expand VACUUM
> syntax and relatively easily can remove this parameter if it's not
> necessary anymore.
>
Attached is a sample patch that controls full page vacuum by new GUC parameter.
Regards,
--
Masahiko Sawada
diff --git a/src/backend/commands/vacuumlazy.c
b/src/backend/commands/vacuumlazy.c
index 784c3e9..03f026d 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -125,6 +125,8 @@ typedef struct LVRelStats
boollock_waiter_detected;
} LVRelStats;
+/* GUC parameter */
+bool vacuum_even_frozen_page; /* should we scan all pages forcibly? */
/* A few variables that don't seem worth passing around as parameters */
static int elevel = -1;
@@ -138,7 +140,7 @@ static BufferAccessStrategy vac_strategy;
/* non-export function prototypes */
static void lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
- Relation *Irel, int nindexes, bool aggressive);
+ Relation *Irel, int nindexes, bool aggressive, bool
scan_all);
static void lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats);
static bool lazy_check_needs_freeze(Buffer buf, bool *hastup);
static void lazy_vacuum_index(Relation indrel,
@@ -246,7 +248,8 @@ lazy_vacuum_rel(Relation onerel, int options, VacuumParams
*params,
vacrelstats->hasindex = (nindexes > 0);
/* Do the vacuuming */
- lazy_scan_heap(onerel, vacrelstats, Irel, nindexes, aggressive);
+ lazy_scan_heap(onerel, vacrelstats, Irel, nindexes, aggressive,
+ vacuum_even_frozen_page);
/* Done with indexes */
vac_close_indexes(nindexes, Irel, NoLock);
@@ -261,7 +264,7 @@ lazy_vacuum_rel(Relation onerel, int options, VacuumParams
*params,
if ((vacrelstats->scanned_pages + vacrelstats->frozenskipped_pages)
< vacrelstats->rel_pages)
{
- Assert(!aggressive);
+ Assert(!aggressive && !vacuum_even_frozen_page);
scanned_all_unfrozen = false;
}
else
@@ -442,7 +445,7 @@ vacuum_log_cleanup_info(Relation rel, LVRelStats
*vacrelstats)
*/
static void
lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
- Relation *Irel, int nindexes, bool aggressive)
+ Relation *Irel, int nindexes, bool aggressive, bool
scan_all)
{
BlockNumber nblocks,
blkno;
@@ -513,6 +516,10 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
* such pages do not need freezing and do not affect the value that we
can
* safely set for relfrozenxid or relminmxid.
*
+* When scan_all is set, we have to scan all pages forcibly while
ignoring
+* visibility map status, and then we can safely set for relfrozenxid or
+* relminmxid.
+*
* Before entering the main loop, establish the invariant that
* next_unskippable_block is the next block number >= blkno that's not
we
* can't skip based on the visibility map, either all-visible for a
@@ -639,11 +646,12 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
/*
* The current block is potentially skippable; if we've
seen a
* long enough run of skippable blocks to justify
skipping it, and
-* we're not forced to check it, then go ahead and skip.
-* Otherwise, the page must be at least all-visible if
not
-* all-froze
Re: [HACKERS] Reviewing freeze map code
On Mon, Jun 6, 2016 at 5:44 PM, Masahiko Sawada wrote: > On Sat, Jun 4, 2016 at 1:46 PM, Masahiko Sawada wrote: >> On Sat, Jun 4, 2016 at 12:59 AM, Robert Haas wrote: >>> On Fri, Jun 3, 2016 at 11:21 AM, Masahiko Sawada >>> wrote: > Can you submit that part as a separate patch? Attached. >>> >>> Thanks, committed. >>> >> I'm address the review comment of 7087166 commit, and will post the >> patch. > > When? On Saturday. >>> >>> Great. Will that address everything for this open item, then? >>> >> >> Attached patch for commit 7087166 on another mail. >> I think that only the test tool for visibility map is remaining and >> under the discussion. >> Even if we have verification tool or function for visibility map, we >> cannot repair the contents of visibility map if we turned out that >> contents of visibility map is something wrong. >> So I think we should have the way that re-generates the visibility map. >> For this purpose, doing vacuum while ignoring visibility map by a new >> option or new function is one idea. >> But IMHO, it's not good idea to allow a function to do vacuum, and >> expanding the VACUUM syntax might be somewhat overkill. >> >> So other idea is to have GUC parameter, vacuum_even_frozen_page for example. >> If this parameter is set true (false by default), we do vacuum whole >> table forcibly and re-generate visibility map. >> The advantage of this idea is that we don't necessary to expand VACUUM >> syntax and relatively easily can remove this parameter if it's not >> necessary anymore. >> > > Attached is a sample patch that controls full page vacuum by new GUC > parameter. Don't we want a reloption for that? Just wondering... -- 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] Reviewing freeze map code
On Mon, Jun 6, 2016 at 5:11 AM, Michael Paquier wrote: >> Attached is a sample patch that controls full page vacuum by new GUC >> parameter. > > Don't we want a reloption for that? Just wondering... Why? Just for consistency? I think the bigger question here is whether we need to do anything at all. It's true that, without some new option, we'll lose the ability to forcibly vacuum every page in the relation, even if all-frozen. But there's not much use case for that in the first place. It will be potentially helpful if it turns out that we have a bug that sets the all-frozen bit on pages that are not, in fact, all-frozen. Otherwise, what's the use? -- 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] Parallel pg_dump's error reporting doesn't work worth squat
How about silencing the workers on termination?
# Build on Windows (with VC?) is very time consuming...
At Fri, 03 Jun 2016 09:44:30 -0400, Tom Lane wrote in
<11515.1464961...@sss.pgh.pa.us>
> Kyotaro HORIGUCHI writes:
> > For sure, any of the "dangers" of TerminateThread don't matter
> > for this case.
>
> I think that this one:
>
> >> If the target thread is allocating memory from the heap, the heap
> >> lock will not be released.
>
> is potentially a hazard, which is why I made sure to use write_stderr
> later on in the console interrupt handler. Your original suggestion
> to use write_msg would end up going through fprintf, which might well
> use malloc internally. (It's possible that Windows' version of write()
> could too, I suppose, but that's probably as low-level as we are
> going to get.)
I have to admit that I forgot about the possible malloc's, and
PQcancel() can be blocked from the same reason. What is worse,
even if we managed to avoid this particular disaster, the MSDN
page says the problems are *for example*. So if we don't allow
any possible unknown blocking on ctrl-C termination and want to
forcibly terminate workers, we might have to use process instead
of thread for worker entity. (This might reduce the difference
related to WIN32..)
We also might be able to cause immediate process termination for
the second Ctrl-C but this seems to lead to other issues.
If the issue to be settled here is the unwanted error messages,
we could set a flag to instruct write_msg to sit silent. Anyway
the workers should have been dead that time so discarding any
error messages don't matter.
What do you think about this?
Timeout for select still seems to be needless.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index fb48a02..b74a396 100644
--- a/src/bin/pg_dump/parallel.c
+++ b/src/bin/pg_dump/parallel.c
@@ -147,7 +147,11 @@ static DWORD tls_index;
/* globally visible variables (needed by exit_nicely) */
bool parallel_init_done = false;
-DWORD mainThreadId;
+bool is_terminating = false;
+static DWORD mainThreadId;
+static bool handler_active = false;
+static DWORD handlerThreadId;
+
#endif /* WIN32 */
static const char *modulename = gettext_noop("parallel archiver");
@@ -180,9 +184,26 @@ static char *readMessageFromPipe(int fd);
/*
- * Shutdown callback to clean up socket access
+ * Return true if I am the main thread
+ *
+ * This function regards the console handler thread as the main thread. We
+ * need a separate boolean for validity of the handlerThreadId since an
+ * invalid value for thread id is not defined.
*/
#ifdef WIN32
+bool
+am_main_thread(void)
+{
+ DWORD current_thread_id = GetCurrentThreadId();
+
+ return (mainThreadId == current_thread_id ||
+ (handler_active &&
+ handlerThreadId == current_thread_id));
+}
+
+/*
+ * Shutdown callback to clean up socket access
+ */
static void
shutdown_parallel_dump_utils(int code, void *unused)
{
@@ -190,7 +211,7 @@ shutdown_parallel_dump_utils(int code, void *unused)
if (mainThreadId == GetCurrentThreadId())
WSACleanup();
}
-#endif
+#endif /* ifdef WIN32 */
/*
* Initialize parallel dump support --- should be called early in process
@@ -390,6 +411,8 @@ ShutdownWorkersHard(ParallelState *pstate)
* On Windows, send query cancels directly to the workers' backends. Use
* a critical section to ensure worker threads don't change state.
*/
+ is_terminating = true; /* Silence workers */
+
EnterCriticalSection(&signal_info_lock);
for (i = 0; i < pstate->numWorkers; i++)
{
@@ -602,6 +625,15 @@ consoleHandler(DWORD dwCtrlType)
if (dwCtrlType == CTRL_C_EVENT ||
dwCtrlType == CTRL_BREAK_EVENT)
{
+ /*
+ * We don't forcibly terminate workes. Instead, these
+ * variables make them silent ever after. This handler thread
+ * is regarded as the main thread.
+ */
+ is_terminating = true;
+ handlerThreadId = GetCurrentThreadId();
+ handler_active = true;
+
/* Critical section prevents changing data we look at here */
EnterCriticalSection(&signal_info_lock);
@@ -642,6 +674,8 @@ consoleHandler(DWORD dwCtrlType)
write_msg(NULL, "terminated by user\n");
}
+ handler_active = false;
+
/* Always return FALSE to allow signal handling to continue */
return FALSE;
}
diff --git a/src/bin/pg_dump/parallel.h b/src/bin/pg_dump/parallel.h
index 21739ca..59fba33 100644
--- a/src/bin/pg_dump/parallel.h
+++ b/src/bin/pg_dump/parallel.h
@@ -64,11 +64,11 @@ typedef struct ParallelState
#ifdef WIN32
extern bool parallel_init_done;
-extern DWORD mainThreadId;
+extern bool is_terminating;
#endif
extern void init_parallel_dump_utils(void);
-
+extern bool am_main_thread(void);
extern int GetIdleWorker(ParallelState *pstate);
extern bool IsEveryWorkerIdle(ParallelState *pstate);
extern void ListenToWorkers(ArchiveHandle *AH, ParallelState *pstate, bool do_wait);
diff --git a/src/bin/pg
Re: [HACKERS] [sqlsmith] Failed assertion in parallel worker (ExecInitSubPlan)
On Sat, Jun 4, 2016 at 1:53 AM, Robert Haas wrote: > > On Thu, May 12, 2016 at 2:07 PM, Tom Lane wrote: > >> Err, wow. That makes my head hurt. Can you explain why this case > >> only arises for appendrel children, and not for plain rels? > > > > Well, plain rels only output Vars ;-) > > Hmm. Dilip's example in > https://www.postgresql.org/message-id/CAFiTN-vzg5BkK6kAh3OMhvgRu-uJvkjz47ybtopMAfGJp=z...@mail.gmail.com > seems to show that it's possible to end up with a targetlist > containing non-Vars even for a baserel. In that example, > pull_up_subqueries() injects t2 into the parent query with a target > list containing two items, one of which is a PlaceHolderVar > referencing the subplan for t3. > > Unfortunately, that makes it a bit hard to avoid wasting cycles here. > Amit's patch in > https://www.postgresql.org/message-id/CAA4eK1L-Uo=s4=0jvvva51pj06u5wddvsqg673yuxj_ja+x...@mail.gmail.com > is a bit brute force: it just checks the target list for every > relation for parallel-restricted constructs. In most cases, no > subqueries having been pulled up, it will find none, but it'll still > have to walk the whole target list to figure that out. If we had some > way of knowing that nothing was pulled up into the current rel, we > could avoid that. This seems to apply equally to baserels and > appendrels; > That seems doable, as for such rels we can only have Vars and PlaceHolderVars in targetlist. Basically, whenever we are adding PlaceHolderVars to a relation, just remember that information and use it later. The patch doing the same is attached with this mail. Now still, this won't cover the case of ChildRels for an Append Relation as for that we adjust target list separately in set_append_rel_size. I think we need to deal with it separately. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com prohibit_parallel_clause_below_rel_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reviewing freeze map code
On Mon, Jun 6, 2016 at 6:34 PM, Robert Haas wrote: > On Mon, Jun 6, 2016 at 5:11 AM, Michael Paquier > wrote: >>> Attached is a sample patch that controls full page vacuum by new GUC >>> parameter. >> >> Don't we want a reloption for that? Just wondering... > > Why? Just for consistency? I think the bigger question here is > whether we need to do anything at all. It's true that, without some > new option, we'll lose the ability to forcibly vacuum every page in > the relation, even if all-frozen. But there's not much use case for > that in the first place. It will be potentially helpful if it turns > out that we have a bug that sets the all-frozen bit on pages that are > not, in fact, all-frozen. Otherwise, what's the use? > I cannot agree with using this parameter as a reloption. We set it true only when the serious bug is discovered and we want to re-generate the visibility maps of specific tables. I thought that control by GUC parameter would be convenient rather than adding the new option. Regards, -- Masahiko Sawada -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] OUT parameter and RETURN table/setof
Hi Is there any option in PGPLSQL which can RETURNS table or SETOF rows along with an OUT parameter? please Thanks Sridhar OpenText
Re: [HACKERS] Reviewing freeze map code
On Sat, Jun 4, 2016 at 12:18 AM, Masahiko Sawada wrote: > Attached updated patch. The error-checking enhancements here look good to me, except that you forgot to initialize totalBytesRead. I've committed those changes with a fix for that problem and will look at the rest of this separately. -- 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] Typo in parallel comment of heap_delete()
On Sun, Jun 5, 2016 at 4:39 PM, Jim Nasby wrote: > I'm pretty sure this is a typo... Sure is. Thanks. -- 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] OUT parameter and RETURN table/setof
On Mon, Jun 6, 2016 at 7:17 AM, Sridhar N Bamandlapally < sridhar@gmail.com> wrote: > Hi > > Is there any option in PGPLSQL which can RETURNS table or SETOF rows along > with an OUT parameter? > > No, there would be no point given the internals of how functions work. What is it you are trying to do? David J.
Re: [HACKERS] Reviewing freeze map code
Masahiko Sawada writes: > On Sat, Jun 4, 2016 at 1:46 PM, Masahiko Sawada wrote: >> So other idea is to have GUC parameter, vacuum_even_frozen_page for example. >> If this parameter is set true (false by default), we do vacuum whole >> table forcibly and re-generate visibility map. >> The advantage of this idea is that we don't necessary to expand VACUUM >> syntax and relatively easily can remove this parameter if it's not >> necessary anymore. > Attached is a sample patch that controls full page vacuum by new GUC > parameter. I find this approach fairly ugly ... it's randomly inconsistent with other VACUUM parameters for no very defensible reason. Taking out GUCs is not easier than taking out statement parameters; you risk breaking applications either way. 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] Relax requirement for INTO with SELECT in pl/pgsql
On Sat, Jun 4, 2016 at 10:45 PM, David G. Johnston
wrote:
> On Tuesday, March 22, 2016, Merlin Moncure wrote:
>>
>>
>> Anyways, here's the patch with documentation adjustments as promised.
>> I ended up keeping the 'without result' section because it contained
>> useful information about plan caching,
>>
>> merlin
>>
>> diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
>> new file mode 100644
>> index 9786242..512eaa7
>> *** a/doc/src/sgml/plpgsql.sgml
>> --- b/doc/src/sgml/plpgsql.sgml
>> *** my_record.user_id := 20;
>> *** 904,910
>> Executing a Command With No Result
>>
>>
>> ! For any SQL command that does not return rows, for example
>>INSERT without a RETURNING clause, you can
>>execute the command within a PL/pgSQL
>> function
>>just by writing the command.
>> --- 904,910
>> Executing a Command With No Result
>>
>>
>> ! For any SQL command, for example
>>INSERT without a RETURNING clause, you can
>>execute the command within a PL/pgSQL
>> function
>>just by writing the command.
>> *** my_record.user_id := 20;
>
>
> This just seems odd...a bit broader approach here would be nice. Especially
> since now it's not the command that doesn't have a result but the user
> decision to not capture any result that may be present. Using insert
> returning for the example here is just, again, odd...
>
> Removing PERFORM requires a bit more reframing of the docs than you've done
> here; too much was influenced by its presence.
>
>>
>> *** 925,972
>>.
>>
>>
>> -
>> - Sometimes it is useful to evaluate an expression or
>> SELECT
>> - query but discard the result, for example when calling a function
>> - that has side-effects but no useful result value. To do
>> - this in PL/pgSQL, use the
>> - PERFORM statement:
>> -
>> -
>> - PERFORM query;
>> -
>> -
>> - This executes query and discards the
>> - result. Write the query the same
>> - way you would write an SQL SELECT command, but replace
>> the
>> - initial keyword SELECT with PERFORM.
>> - For WITH queries, use PERFORM and then
>> - place the query in parentheses. (In this case, the query can only
>> - return one row.)
>> - PL/pgSQL variables will be
>> - substituted into the query just as for commands that return no
>> result,
>> - and the plan is cached in the same way. Also, the special variable
>> - FOUND is set to true if the query produced at
>> - least one row, or false if it produced no rows (see
>> - ).
>> -
>> -
>>
>>
>> ! One might expect that writing SELECT directly
>> ! would accomplish this result, but at
>> ! present the only accepted way to do it is
>> ! PERFORM. A SQL command that can return rows,
>> ! such as SELECT, will be rejected as an error
>> ! unless it has an INTO clause as discussed in the
>> ! next section.
>>
>>
>>
>>
>>An example:
>>
>> ! PERFORM create_mv('cs_session_page_requests_mv', my_query);
>>
>>
>>
>> --- 925,944
>>.
>>
>>
>>
>>
>> ! In older versions of PostgreSQL, it was mandatory to use
>> ! PERFORM instead of SELECT
>> ! when the query could return data that was not captured into
>> ! variables. This requirement has been relaxed and usage of
>> ! PERFORM has been deprecated.
>>
>>
>>
>>
>>An example:
>>
>> ! SELECT create_mv('cs_session_page_requests_mv', my_query);
>
>
> I'd consider making the note into a paragraph and then saying that the
> select form and perform form are equivalent by writing both out one after
> the other. The note brings emphasis that is not necessary if we want to
> de-emphasize perform.
>
>>
>>
>>
>> *** GET DIAGNOSTICS integer_var = ROW_COUNT;
>> *** 1480,1491
>>
>>
>>
>> ! A PERFORM statement sets
>> FOUND
>> true if it produces (and discards) one or more rows, false
>> if
>> no row is produced.
>>
>>
>>
>>
>> UPDATE, INSERT, and
>> DELETE
>> statements set FOUND true if at least one
>> --- 1452,1471
>>
>>
>>
>> ! A SELECT statement sets FOUND
>> true if it produces (and discards) one or more rows, false
>> if
>> no row is produced.
>>
>
>
> This could be worded better. It will return true regardless of whether the
> result is discarded.
>
>>
>>
>>
>> +
>> + A PERFORM statement sets
>> FOUND
>> + true if it produces (and discards) one or more rows, false
>> if
>> + no row
Re: [HACKERS] Reviewing freeze map code
On Mon, Jun 6, 2016 at 7:46 AM, Robert Haas wrote: > On Sat, Jun 4, 2016 at 12:18 AM, Masahiko Sawada > wrote: >> Attached updated patch. > > The error-checking enhancements here look good to me, except that you > forgot to initialize totalBytesRead. I've committed those changes > with a fix for that problem and will look at the rest of this > separately. Committed that now, too. -- 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] Reviewing freeze map code
On Mon, Jun 6, 2016 at 9:53 AM, Tom Lane wrote: > Masahiko Sawada writes: >> On Sat, Jun 4, 2016 at 1:46 PM, Masahiko Sawada >> wrote: >>> So other idea is to have GUC parameter, vacuum_even_frozen_page for example. >>> If this parameter is set true (false by default), we do vacuum whole >>> table forcibly and re-generate visibility map. >>> The advantage of this idea is that we don't necessary to expand VACUUM >>> syntax and relatively easily can remove this parameter if it's not >>> necessary anymore. > >> Attached is a sample patch that controls full page vacuum by new GUC >> parameter. > > I find this approach fairly ugly ... it's randomly inconsistent with other > VACUUM parameters for no very defensible reason. Just to be sure I understand, in what way is it inconsistent? > Taking out GUCs is not > easier than taking out statement parameters; you risk breaking > applications either way. Agreed, but that doesn't really answer the question of which one we should have, if either. My gut feeling on this is to either do nothing or add a VACUUM option (not a GUC, not a reloption) called even_frozen_pages, default false. What is your opinion? -- 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] installcheck failing on psql_crosstab
Michael Paquier wrote: > I know that we guarantee that make installcheck may not work on many > target servers as a lot of tests are very GUC-sensitive, but this > looks a bit oversensitive to me, especially knowing that it is the > only diff generated by the whole test suite. > Don't you think that those tests could be made more portable? Not sure about that. I think the only way to change this would be to remove those particular tests completely, and I don't think that's a good tradeoff. If somebody can show a way to avoid the problem without removing those tests for multiline functionality, I'm all ears. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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] installcheck failing on psql_crosstab
Alvaro Herrera writes: > Michael Paquier wrote: >> I know that we guarantee that make installcheck may not work on many >> target servers as a lot of tests are very GUC-sensitive, but this >> looks a bit oversensitive to me, especially knowing that it is the >> only diff generated by the whole test suite. >> Don't you think that those tests could be made more portable? > Not sure about that. I think the only way to change this would be to > remove those particular tests completely, and I don't think that's a > good tradeoff. If somebody can show a way to avoid the problem without > removing those tests for multiline functionality, I'm all ears. Presumably what is happening is that the planner is switching from hash to sort aggregation. We could force the plan choice via enable_hashagg, which seems OK from the standpoint that this is only meant to test psql not the backend. However, I'm dubious of the entire project. I think if you push the value of work_mem a bit further in either direction, you will find other tests falling over. I'm not excited about changing this one just because it's currently the first to fail. 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] Reviewing freeze map code
Robert Haas writes: > On Mon, Jun 6, 2016 at 9:53 AM, Tom Lane wrote: >> Taking out GUCs is not >> easier than taking out statement parameters; you risk breaking >> applications either way. > Agreed, but that doesn't really answer the question of which one we > should have, if either. My gut feeling on this is to either do > nothing or add a VACUUM option (not a GUC, not a reloption) called > even_frozen_pages, default false. What is your opinion? That's about where I stand, with some preference for "do nothing". I'm not convinced we need this. 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] Reviewing freeze map code
Robert Haas wrote: > My gut feeling on this is to either do nothing or add a VACUUM option > (not a GUC, not a reloption) called even_frozen_pages, default false. > What is your opinion? +1 for that approach -- I thought that was already agreed weeks ago and the only question was what to name that option. even_frozen_pages sounds better than SCANALL, SCAN_ALL, FREEZE, FORCE (the other options I saw proposed in that subthread), so +1 for that naming too. I don't like doing nothing; that means that when we discover a bug we'll have to tell users to rm a file whose name requires a complicated catalog query to find out, so -1 for that. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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] Reviewing freeze map code
On Fri, Jun 3, 2016 at 11:41 PM, Robert Haas wrote: > (Status update for Noah: I expect Masahiko Sawada will respond > quickly, but if not I'll give some kind of update by Monday COB > anyhow.) I believe this open item is now closed, unless Andres has more comments or wishes to discuss any point further, with the exception that we still need to decide whether to add VACUUM (even_frozen_pages) or some variant of that. I have added a new open item for that issue and marked this one as resolved. My intended strategy as the presumptive owner of the new items is to do nothing unless more of a consensus emerges than we have presently. We do not seem to have clear agreement on whether to add the new option; whether to make it a GUC, a reloption, a VACUUM syntax option, or some combination of those things; and whether it should blow up the existing VM and rebuild it (as proposed by Sawada-san) or just force frozen pages to be scanned in the hope that something good will happen (as proposed by Andres). In the absence of consensus, doing nothing is a reasonable choice here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] installcheck failing on psql_crosstab
Tom Lane wrote: > Alvaro Herrera writes: > > Michael Paquier wrote: > >> I know that we guarantee that make installcheck may not work on many > >> target servers as a lot of tests are very GUC-sensitive, but this > >> looks a bit oversensitive to me, especially knowing that it is the > >> only diff generated by the whole test suite. > >> Don't you think that those tests could be made more portable? > > > Not sure about that. I think the only way to change this would be to > > remove those particular tests completely, and I don't think that's a > > good tradeoff. If somebody can show a way to avoid the problem without > > removing those tests for multiline functionality, I'm all ears. > > Presumably what is happening is that the planner is switching from hash > to sort aggregation. We could force the plan choice via enable_hashagg, > which seems OK from the standpoint that this is only meant to test psql > not the backend. However, I'm dubious of the entire project. I think > if you push the value of work_mem a bit further in either direction, > you will find other tests falling over. I'm not excited about changing > this one just because it's currently the first to fail. I can't imagine that the server is avoiding hash aggregation on a 1MB work_mem limit for data that's a few dozen of bytes. Is it really doing that? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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] COMMENT ON, psql and access methods
Noah Misch wrote: > The above-described topic is currently a PostgreSQL 9.6 open item. Álvaro, > since you committed the patch believed to have created it, you own this open > item. If some other commit is more relevant or if this does not belong as a > 9.6 open item, please let us know. Otherwise, please observe the policy on > open item ownership[1] and send a status update within 72 hours of this > message. Include a date for your subsequent status update. Testers may > discover new open items at any time, and I want to plan to get them all fixed > well in advance of shipping 9.6rc1. Consequently, I will appreciate your > efforts toward speedy resolution. Thanks. I'll have this patch reviewed, and barring serious problems, committed no later than EOB Friday 10th. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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] installcheck failing on psql_crosstab
Alvaro Herrera writes: > Tom Lane wrote: >> Presumably what is happening is that the planner is switching from hash >> to sort aggregation. > I can't imagine that the server is avoiding hash aggregation on a 1MB > work_mem limit for data that's a few dozen of bytes. Is it really doing > that? Yup: regression=# explain SELECT v,h, string_agg(i::text, E'\n') AS i FROM ctv_data GROUP BY v, h ORDER BY h,v; QUERY PLAN Sort (cost=33.87..34.37 rows=200 width=96) Sort Key: h, v -> HashAggregate (cost=23.73..26.23 rows=200 width=96) Group Key: h, v -> Seq Scan on ctv_data (cost=0.00..16.10 rows=610 width=68) (5 rows) regression=# set work_mem = '1MB'; SET regression=# explain SELECT v,h, string_agg(i::text, E'\n') AS i FROM ctv_data GROUP BY v, h ORDER BY h,v; QUERY PLAN GroupAggregate (cost=44.32..55.97 rows=200 width=96) Group Key: h, v -> Sort (cost=44.32..45.85 rows=610 width=68) Sort Key: h, v -> Seq Scan on ctv_data (cost=0.00..16.10 rows=610 width=68) (5 rows) Now that you mention it, this does seem a bit odd, although I remember that there's a pretty substantial fudge factor in there when we have no statistics (which we don't in this example). If I ANALYZE ctv_data then it sticks to the hashagg plan all the way down to 64kB work_mem. 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] Reviewing freeze map code
On Mon, Jun 6, 2016 at 10:18 AM, Alvaro Herrera wrote: > Robert Haas wrote: >> My gut feeling on this is to either do nothing or add a VACUUM option >> (not a GUC, not a reloption) called even_frozen_pages, default false. >> What is your opinion? > > +1 for that approach -- I thought that was already agreed weeks ago and > the only question was what to name that option. even_frozen_pages > sounds better than SCANALL, SCAN_ALL, FREEZE, FORCE (the other > options I saw proposed in that subthread), so +1 for that naming > too. > > I don't like doing nothing; that means that when we discover a bug we'll > have to tell users to rm a file whose name requires a complicated > catalog query to find out, so -1 for that. So... I agree that it is definitely not good if we have to tell users to rm a file, but I am not quite sure how this new option would prevent us from having to say that? Here are some potential kinds of bugs we might have: 1. Sometimes, the all-frozen bit doesn't get set when it should. 2. Sometimes, the all-frozen bit gets sit when it shouldn't. 3. Some combination of (1) and (2), so that the VM fork can't be trusted in either direction. If (1) happens, removing the VM fork is not a good idea; what people will want to do is re-run a VACUUM FREEZE. If (2) or (3) happens, removing the VM fork might be a good idea, but it's not really clear that VACUUM (even_frozen_pages) will help much. For one thing, if there are actually unfrozen tuples on those pages and the clog pages which they reference are already gone or recycled, rerunning VACUUM on the table in any form might permanently lose data, or maybe it will just fail. If because of the nature of the bug you somehow know that case doesn't pertain, then I suppose the bug is that the tuple-level and page-level state is out of sync. VACUUM (even_frozen_pages) probably won't help with that much either, because VACUUM never clears the all-frozen bit without also clearing the all-visible bit, and that only if the page contains dead tuples, which in this case it probably doesn't. I'm intuitively sympathetic to the idea that we should have an option for this, but I can't figure out in what case we'd actually tell anyone to use it. It would be useful for the kinds of bugs listed above to have VACUUM (rebuild_vm) to blow away the VM fork and rebuild it, but that's different semantics than what we proposed for VACUUM (even_frozen_pages). And I'd be sort of inclined to handle that case by providing some other way to remove VM forks (like a new function in the pg_visibilitymap contrib module, maybe?) and then just tell people to run regular VACUUM afterwards, rather than putting the actual VM fork removal into VACUUM. -- 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] TESTING in src/bin/pg_upgrade has incorrect documentation
On Thu, Jun 2, 2016 at 12:35 PM, Andreas 'ads' Scherbaum wrote: > the TESTING file in src/bin/pg_upgrade talks about a "check.sh script", but > this seems to be a binary (check) now. Nope: [rhaas pgsql]$ git grep -F check.sh [rhaas pgsql]$ -- 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] Let file_fdw access COPY FROM PROGRAM
On Thu, Jun 2, 2016 at 4:48 PM, Corey Huinker wrote: > A while back, there was a push to make COPY gzip-aware. That didn't happen, > but COPY FROM PROGRAM did, and it scratches the same itch. > > I have a similar need, but with file_fdw foreign tables. I have .csv.gz > files downloaded to the server, but those CSVs have 100+ columns in them, > and in this case I only really care about a half dozen of those columns. I'd > like to avoid: > - the overhead of writing the uncompressed file to disk and then immediately > re-reading it > - writing unwanted columns to a temp/work table via COPY, and then > immediately re-reading them > - multicorn fdw because it ends up making a python string out of all data > cells > - a csv parsing tool like csvtool or mlr, because they output another CSV > which must be reparsed from scratch > > Since file_fdw leverages COPY, it seemed like it would be easy to add the > FROM PROGRAM feature to file_fdw. I began asking questions on #postgresql > IRC, only to discover that Adam Gomaa ( akgo...@gmail.com ) had already > written such a thing, but hadn't submitted it. Attached is a small rework of > his patch, along with documentation. His failure to submit that here himself raises the question of whether he is OK with that code being released under the PostgreSQL license. If this patch is going to be considered, I think we should have a post from him clarifying that matter. -- 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] Rename synchronous_standby_names?
On Tue, May 31, 2016 at 2:19 PM, Peter Eisentraut wrote: > On 5/31/16 1:47 PM, Jaime Casanova wrote: >> >> Are we going to change synchronous_standby_names? Certainly the GUC is >> not *only* a list of names anymore. >> >> synchronous_standby_config? >> synchronous_standbys (adjust to correct english if necesary)? > > > If the existing values are still going to be accepted, then I would leave it > as is. +1, emphatically. -- 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] Reviewing freeze map code
Robert Haas writes: > I'm intuitively sympathetic to the idea that we should have an option > for this, but I can't figure out in what case we'd actually tell > anyone to use it. It would be useful for the kinds of bugs listed > above to have VACUUM (rebuild_vm) to blow away the VM fork and rebuild > it, but that's different semantics than what we proposed for VACUUM > (even_frozen_pages). And I'd be sort of inclined to handle that case > by providing some other way to remove VM forks (like a new function in > the pg_visibilitymap contrib module, maybe?) and then just tell people > to run regular VACUUM afterwards, rather than putting the actual VM > fork removal into VACUUM. There's a lot to be said for that approach. If we do it, I'd be a bit inclined to offer an option to blow away the FSM as well. 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] Problem with dumping bloom extension
On Fri, Jun 3, 2016 at 12:31 PM, Stephen Frost wrote: >> Stephen, something is smelling wrong in checkExtensionMembership() >> since 5d58999, an access method does not have ACLs and I would have >> expected that when this routine is invoked in >> selectDumpableAccessMethod() the object is not selected as dumpable. > > Yeah, I saw this also and was going to look into it. > > I suspect the issue may actually be that dumpAccessMethod() wasn't ever > updated to have the appropriate conditionals for each of the components > of the object. > > Specifically, there should be if statements along the lines of: > > if (aminfo->dobj.dump & DUMP_COMPONENT_DEFINITION) > ArchiveEntry ... > > if (aminfo->dobj.dump & DUMP_COMPONENT_COMMENT) > dumpComment() > > towards the end of dumpAccessMethod(). > > I'm not 100% sure that addresses this, but it definitely needs to be > fixed also. I'll take care of it in the next few days. > > I'll also look more directly into what's going on here this weekend when > I've got a bit more time to do so. It seems important to get this fixed. I added it to the open items list. -- 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
[HACKERS] Improve tab completion for USER MAPPING
Hi all, I found that the tab completion for USER MAPPING doesn't work fine. For example, The below is no problem. postgres=# create user[TAB] user user mapping for But this doesn't work fine. (Note that there is a white space between 'user' and [TAB]) postgres=# create user [TAB] hoge_user masahiko pg_signal_backend After manual input of the 'mapping', 'for' is added by tab completion. It means that the tab completion for 'mapping' is not working. Patch attached. Please review it. Regards, -- Masahiko Sawada fix_tab_completion_for_user_mapping.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] Parallel pg_dump's error reporting doesn't work worth squat
Kyotaro HORIGUCHI writes: > At Fri, 03 Jun 2016 09:44:30 -0400, Tom Lane wrote in > <11515.1464961...@sss.pgh.pa.us> >> I think that this one: >>> If the target thread is allocating memory from the heap, the heap >>> lock will not be released. >> is potentially a hazard, which is why I made sure to use write_stderr >> later on in the console interrupt handler. Your original suggestion >> to use write_msg would end up going through fprintf, which might well >> use malloc internally. (It's possible that Windows' version of write() >> could too, I suppose, but that's probably as low-level as we are >> going to get.) > I have to admit that I forgot about the possible malloc's, and > PQcancel() can be blocked from the same reason. Uh, what? PQcancel is very carefully coded so that it's safe to use in a signal handler. If it's doing mallocs someplace, that would be surprising. > If the issue to be settled here is the unwanted error messages, > we could set a flag to instruct write_msg to sit silent. Anyway > the workers should have been dead that time so discarding any > error messages don't matter. > What do you think about this? This is really ugly and I'm unconvinced that it fixes anything. write_msg is hardly the only place in a worker thread that might be doing malloc's; moreover, preventing workers from entering it after we start a shutdown does nothing for workers that might be in it already. 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
[HACKERS] Why we don't have checksums on clog files
Hello! Why we don't have checksums on clog files. We have checksum on pg_control, optional checksumming on data files, some form of checksumming on wal's. But why we don't have any checksumming on clogs. Corruptions on clogs lead to transaction visisbility problems and database consistency violation. Can anybody explain this situation with clogs? -- Alex Ignatov 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] Changed SRF in targetlist handling
On Mon, May 23, 2016 at 4:15 PM, Tom Lane wrote: > We should consider single and multiple SRFs in a targetlist as distinct > use-cases; only the latter has got weird properties. > > There are several things we could potentially do with multiple SRFs in > the same targetlist. In increasing order of backwards compatibility and > effort required: > > 1. Throw error if there's more than one SRF. > > 2. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...). This would > have the same behavior as before if the SRFs all return the same number > of rows, and otherwise would behave differently. I thought the idea was to rewrite it as LATERAL ROWS FROM (srf1()), LATERAL ROWS FROM (srf2()), ... The rewrite you propose here seems to NULL-pad rows after the first SRF is exhausted: rhaas=# select * from dual, lateral rows from (generate_series(1,3), generate_series(1,4)); x | generate_series | generate_series ---+-+- dummy | 1 | 1 dummy | 2 | 2 dummy | 3 | 3 dummy | | 4 (4 rows) ...whereas with a separate LATERAL clause for each row you get this: rhaas=# select * from dual, lateral rows from (generate_series(1,3)) a, lateral rows from (generate_series(1,4)) b; x | a | b ---+---+--- dummy | 1 | 1 dummy | 1 | 2 dummy | 1 | 3 dummy | 1 | 4 dummy | 2 | 1 dummy | 2 | 2 dummy | 2 | 3 dummy | 2 | 4 dummy | 3 | 1 dummy | 3 | 2 dummy | 3 | 3 dummy | 3 | 4 (12 rows) The latter is how I'd expect SRF-in-targetlist to work. -- 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] Reviewing freeze map code
On 2016-06-06 11:37:25 -0400, Robert Haas wrote: > On Mon, Jun 6, 2016 at 11:28 AM, Andres Freund wrote: > > Except that we right now don't have any realistic way to figure out > > whether this new feature actually does the right thing. Which makes > > testing this *considerably* harder than just VACUUM (dwim). I think it's > > unacceptable to release this feature without a way that'll tell that it > > so far has/has not corrupted the database. Would that, in a perfect > > world, be vacuum? No, probably not. But since we're not in a perfect > > world... > > I just don't see how running VACUUM on the all-frozen pages is going > to help. Because we can tell people in the beta2 announcement or some wiki page "please run VACUUM(scan_all)" and check whether it emits WARNINGs. And if we suspect freeze map in bug reports, we can just ask reporters to run a VACUUM (scan_all). > In terms of diagnostic tools, you can get the VM bits and > page-level bits using the pg_visibility extension; I wrote it > precisely because of concerns like the ones you raise here. If you > want to cross-check the page-level bits against the tuple-level bits, > you can do that with the pageinspect extension. And if you do those > things, you can actually find out whether stuff is broken. That's WAY out ouf reach of any "normal users". Adding a vacuum option is doable, writing complex queries is not. > Vacuuming the all-frozen pages won't tell you that. It will either do > nothing (which doesn't tell you that things are OK) or it will change > something (possibly without reporting any message, and possibly making > a bad situation worse instead of better). We found a number of bugs for the equivalent issues in all-visible handling via the vacuum error reporting around those. Greetings, Andres Freund -- 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] Prepared statements and generic plans
On Mon, Jun 6, 2016 at 07:19:37AM +, Albe Laurenz wrote: > Bruce Momjian wrote: > > OK, updated version attached. I added "potential" to the first > > paragraph, and added "estimated cost" to the later part, fixed the > > "cheaper than", and clarified that we add the plan time cost to the > > non-generic plan, which is how it can be cheaper than the generic plan. > > I also moved the "Once a generic plan is chosen" line. > > > > Yeah, that's a lot of changes, but they all improved the text. Thanks. > > Thanks for working on this. > > !Prepared statements can optionally use generic plans rather than > !re-planning with each set of supplied EXECUTE values. > !This occurs immediately for prepared statements with no parameters; > !otherwise it occurs only after five or more executions produce estimated > !plan costs, with planning overhead added, that are, on average, more > !expensive than the generic plan cost. > > The following might be easier to understand: > ... after five or more executions produce plans whose estimated cost average > (including planning overhead) is more expensive than the generic plan cost > estimate. Agreed. > !A generic plan assumes each value supplied to EXECUTE > > ... assumes *that* each value ... Agreed. > !is one of the column's distinct values and that column values are > !uniformly distributed. For example, if statistics records three > > Shouldn't it be "record"? > The documentation treats "statistics" as a plural word throughout. Agreed, not sure how I missed that. > !distinct column values, a generic plan assumes a column equality > !comparison will match 33% of processed rows. Column statistics > > ... assumes *that* a column equality comparison will match 33% of *the* > processed rows. Uh, that seems overly wordy. I think the rule is that if the sentence makes sense without the words, you should not use them, but it is clearly a judgement call in this case. Do you agree? > !also allows generic plans to accurately compute the selectivity of > > Column statistics also *allow* ... Yep. Updated patch attached. One more thing --- there was talk of moving some of this into chapter 66, but as someone already mentioned, there are no subsections there because it is a dedicated topic: 66. How the Planner Uses Statistics. I am not inclined to add a prepare-only section to that chapter. On the other hand, the issues described apply to PREPARE and to protocol-level prepare, so having it in PREPARE also seems illogical. However, I am inclined to leave it in PREPARE until we are ready to move all of this to chapter 66. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription + diff --git a/doc/src/sgml/ref/prepare.sgml b/doc/src/sgml/ref/prepare.sgml new file mode 100644 index dbce8f2..1bf9179 *** a/doc/src/sgml/ref/prepare.sgml --- b/doc/src/sgml/ref/prepare.sgml *** PREPARE n *** 70,80 !Prepared statements have the largest performance advantage when a !single session is being used to execute a large number of similar statements. The performance difference will be particularly !significant if the statements are complex to plan or rewrite, for !example, if the query involves a join of many tables or requires the application of several rules. If the statement is relatively simple to plan and rewrite but relatively expensive to execute, the performance advantage of prepared statements will be less noticeable. --- 70,80 !Prepared statements potentially have the largest performance advantage !when a single session is being used to execute a large number of similar statements. The performance difference will be particularly !significant if the statements are complex to plan or rewrite, e.g. !if the query involves a join of many tables or requires the application of several rules. If the statement is relatively simple to plan and rewrite but relatively expensive to execute, the performance advantage of prepared statements will be less noticeable. *** PREPARE n *** 127,140 Notes !If a prepared statement is executed enough times, the server may eventually !decide to save and re-use a generic plan rather than re-planning each time. !This will occur immediately if the prepared statement has no parameters; !otherwise it occurs only if the generic plan appears to be not much more !expensive than a plan that depends on specific parameter values. !Typically, a generic plan will be selected only if the query's performance !is estimated to be fairly insensitive to the specific parameter values !supplied. --- 127,151 Notes !Prepared st
Re: [HACKERS] Reviewing freeze map code
On Mon, Jun 6, 2016 at 11:28 AM, Andres Freund wrote: > On 2016-06-06 05:34:32 -0400, Robert Haas wrote: >> On Mon, Jun 6, 2016 at 5:11 AM, Michael Paquier >> wrote: >> >> Attached is a sample patch that controls full page vacuum by new GUC >> >> parameter. >> > >> > Don't we want a reloption for that? Just wondering... >> >> Why? Just for consistency? I think the bigger question here is >> whether we need to do anything at all. It's true that, without some >> new option, we'll lose the ability to forcibly vacuum every page in >> the relation, even if all-frozen. But there's not much use case for >> that in the first place. It will be potentially helpful if it turns >> out that we have a bug that sets the all-frozen bit on pages that are >> not, in fact, all-frozen. Otherwise, what's the use? > > Except that we right now don't have any realistic way to figure out > whether this new feature actually does the right thing. Which makes > testing this *considerably* harder than just VACUUM (dwim). I think it's > unacceptable to release this feature without a way that'll tell that it > so far has/has not corrupted the database. Would that, in a perfect > world, be vacuum? No, probably not. But since we're not in a perfect world... I just don't see how running VACUUM on the all-frozen pages is going to help. In terms of diagnostic tools, you can get the VM bits and page-level bits using the pg_visibility extension; I wrote it precisely because of concerns like the ones you raise here. If you want to cross-check the page-level bits against the tuple-level bits, you can do that with the pageinspect extension. And if you do those things, you can actually find out whether stuff is broken. Vacuuming the all-frozen pages won't tell you that. It will either do nothing (which doesn't tell you that things are OK) or it will change something (possibly without reporting any message, and possibly making a bad situation worse instead of better). -- 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] installcheck failing on psql_crosstab
Tom Lane wrote: > Alvaro Herrera writes: > > I can't imagine that the server is avoiding hash aggregation on a 1MB > > work_mem limit for data that's a few dozen of bytes. Is it really doing > > that? > > Yup: Aha. Thanks for testing. > Now that you mention it, this does seem a bit odd, although I remember > that there's a pretty substantial fudge factor in there when we have > no statistics (which we don't in this example). If I ANALYZE ctv_data > then it sticks to the hashagg plan all the way down to 64kB work_mem. Hmm, so we could solve the complaint by adding an ANALYZE. I'm open to that; other opinions? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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] Changed SRF in targetlist handling
Robert Haas writes: > On Mon, May 23, 2016 at 4:15 PM, Tom Lane wrote: >> 2. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...). This would >> have the same behavior as before if the SRFs all return the same number >> of rows, and otherwise would behave differently. > I thought the idea was to rewrite it as LATERAL ROWS FROM (srf1()), > LATERAL ROWS FROM (srf2()), ... No, because then you get the cross-product of multiple SRFs, not the run-in-lockstep behavior. > The rewrite you propose here seems to NULL-pad rows after the first > SRF is exhausted: Yes. That's why I said it's not compatible if the SRFs don't all return the same number of rows. It seems like a reasonable definition to me though, certainly much more reasonable than the current run-until-LCM behavior. > The latter is how I'd expect SRF-in-targetlist to work. That's not even close to how it works now. It would break *every* existing application that has multiple SRFs in the tlist, not just the ones whose SRFs return different numbers of rows. And I'm not convinced that it's a more useful behavior. 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] Reviewing freeze map code
Robert Haas writes: > On Mon, Jun 6, 2016 at 11:28 AM, Andres Freund wrote: >> Except that we right now don't have any realistic way to figure out >> whether this new feature actually does the right thing. > I just don't see how running VACUUM on the all-frozen pages is going > to help. Yes. I don't see that any of the proposed features would be very useful for answering the question "is my VM incorrect". Maybe they would fix problems, and maybe not, but in any case you couldn't rely on VACUUM to tell you about a problem. (Even if you've got warning messages in there, they might disappear into the postmaster log during an auto-vacuum. Warning messages in VACUUM are not a good debugging technology.) 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] installcheck failing on psql_crosstab
I wrote: > Alvaro Herrera writes: >> Tom Lane wrote: >>> Presumably what is happening is that the planner is switching from hash >>> to sort aggregation. >> I can't imagine that the server is avoiding hash aggregation on a 1MB >> work_mem limit for data that's a few dozen of bytes. Is it really doing >> that? > Yup: I looked more closely and found that the reason it's afraid to use hash aggregation is the amount of transition space potentially needed by string_agg. That's being estimated as 8kB per group, and with the (default) estimate of 200 groups, you get about 1.6MB estimated to be needed. Also, I confirmed my suspicion that some other regression tests fail when you reduce work_mem below 1MB. So I'm not really excited about changing this one. 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] Reviewing freeze map code
On 2016-06-06 05:34:32 -0400, Robert Haas wrote: > On Mon, Jun 6, 2016 at 5:11 AM, Michael Paquier > wrote: > >> Attached is a sample patch that controls full page vacuum by new GUC > >> parameter. > > > > Don't we want a reloption for that? Just wondering... > > Why? Just for consistency? I think the bigger question here is > whether we need to do anything at all. It's true that, without some > new option, we'll lose the ability to forcibly vacuum every page in > the relation, even if all-frozen. But there's not much use case for > that in the first place. It will be potentially helpful if it turns > out that we have a bug that sets the all-frozen bit on pages that are > not, in fact, all-frozen. Otherwise, what's the use? Except that we right now don't have any realistic way to figure out whether this new feature actually does the right thing. Which makes testing this *considerably* harder than just VACUUM (dwim). I think it's unacceptable to release this feature without a way that'll tell that it so far has/has not corrupted the database. Would that, in a perfect world, be vacuum? No, probably not. But since we're not in a perfect world... Andres -- 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] Prepared statements and generic plans
On Mon, Jun 6, 2016 at 11:45 AM, Bruce Momjian *EXTERN* wrote: > > One more thing --- there was talk of moving some of this into chapter > 66, but as someone already mentioned, there are no subsections there > because it is a dedicated topic: > > 66. How the Planner Uses Statistics. > I am not inclined to add a prepare-only section to that chapter. On the > other hand, the issues described apply to PREPARE and to protocol-level > prepare, so having it in PREPARE also seems illogical. However, I am > inclined to leave it in PREPARE until we are ready to move all of this > to chapter 66. > > Actually, 66.1 exists - Row Estimation Examples We should create 66.2 - e.g. Statistics of Parameterized Plans My point was 66. How the Planner Uses Statistics doesn't have a table of contents. I suspect it will if we add 66.2. But, should it have one now, when the only section is 66.1? The wording improvement are good; but my recommended split between the SQL command section and 66.2 still applies. David J.
Re: [HACKERS] New design for FK-based join selectivity estimation
Tomas Vondra writes: > One of the recent issues with the current design was handling of > inheritance / appendrels. ISTM the proposed design has the same issue, > no? What happens if the relations are partitioned? I haven't thought about inheritance in this proposal. My initial feeling is that considering the parent table's outgoing FKs (if any) as valid is not unreasonable. If it has any, probably all the children do too. Not sure about incoming FKs, but there probably are none anyhow, since our implementation doesn't really permit reasonable FK definitions that reference a partitioned table. In any case, whatever we might choose to do differently for inheritance would be no harder in this scheme than what's there now; plus, whatever it is, we'd do it once not once per join relation. 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] Let file_fdw access COPY FROM PROGRAM
I'm fine with the code being released under the PostgreSQL license. Thanks, Adam -- 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] pg9.6 segfault using simple query (related to use fk for join estimates)
Tomas Vondra writes: > On 06/04/2016 08:15 PM, Tom Lane wrote: >> * Make RelationGetFKeyList cache a list of ForeignKeyOptInfo structs, >> not just constraint OIDs. It's insane that the relcache scans >> pg_constraint to collect those OIDs and then the planner re-reads all >> those same rows on every planning cycle. > That seems like a fairly straightforward change, and I'm not opposed to > doing that. However RelationGetFKeyList is basically a modified copy of > RelationGetIndexList, so it shares the same general behavior, including > caching of OIDs and then constructing IndexOptInfo objects on each > get_relation_info() call. Why is it 'insane' for foreign keys but not > for indexes? Or what am I missing? I would not be in favor of migrating knowledge of IndexOptInfo into the relcache; it's too planner-specific. Also, it mostly copies info from the index's relcache entry, not the parent relation's (which for one thing would imply locking hazards if we tried to cache that info in the parent rel). But for foreign keys, we can cache an image of certain well-defined fields of certain well-defined rows of pg_constraint, and that seems like a reasonably arm's-length definition of a responsibility to give the relcache, especially when it has to visit all and only those same rows to construct what it's caching now. >> would be okay if you checked after identifying a matching eclass member >> that it belonged to the FK's referenced table, but AFAICS there is no such >> check anywhere. > Perhaps I'm missing something, but I thought this is checked by these > conditions in quals_match_foreign_key(): > 1) with ECs (line 3990) > if (foreignrel->relid == var->varno && > fkinfo->confkeys[i] == var->varattno) > foundvarmask |= 1; This checks that you found a joinclause mentioning foreignrel. But foreignrel need have nothing to do with the foreign key; it could be any table in the query. That comparison of confkeys[] and varattno is thus checking for column-number equality of two columns that might be from different relations. That is, if we have an FK "A.X references B.Y", and the query contains "A.X = C.Z", this code could match the FK to that clause if Y and Z happen to have the same data types and column numbers. 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] Changed SRF in targetlist handling
On Mon, Jun 6, 2016 at 11:50 AM, Tom Lane wrote: > Robert Haas writes: > > On Mon, May 23, 2016 at 4:15 PM, Tom Lane wrote: > >> 2. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...). This would > >> have the same behavior as before if the SRFs all return the same number > >> of rows, and otherwise would behave differently. > > > I thought the idea was to rewrite it as LATERAL ROWS FROM (srf1()), > > LATERAL ROWS FROM (srf2()), ... > > No, because then you get the cross-product of multiple SRFs, not the > run-in-lockstep behavior. > > > The rewrite you propose here seems to NULL-pad rows after the first > > SRF is exhausted: > > Yes. That's why I said it's not compatible if the SRFs don't all return > the same number of rows. It seems like a reasonable definition to me > though, certainly much more reasonable than the current run-until-LCM > behavior. > IOW, this is why this mode query has to fail. > > > The latter is how I'd expect SRF-in-targetlist to work. > > That's not even close to how it works now. It would break *every* > existing application that has multiple SRFs in the tlist, not just > the ones whose SRFs return different numbers of rows. And I'm not > convinced that it's a more useful behavior. > To clarify, the present behavior is basically a combination of both of Robert's results. If the SRFs return the same number of rows the first (zippered) result is returned without an NULL padding. If the SRFs return a different number of rows the LCM behavior kicks in and you get Robert's second result. SELECT generate_series(1, 4), generate_series(1, 4) ORDER BY 1, 2; is the same as SELECT * FROM ROWS FROM ( generate_series(1, 4), generate_series(1, 4) ); BUT SELECT generate_series(1, 3), generate_series(1, 4) ORDER BY 1, 2; is the same as SELECT * FROM ROWS FROM generate_series(1, 3) a, LATERAL ROWS FROM generate_series(1, 4) b; Tom's 2.5 proposal basically says we make the former equivalence succeed and have the later one fail. The rewrite would be unaware of the cardinality of the SRF and so it cannot conditionally rewrite the query. One of the two must be chosen and the incompatible behavior turned into an error. David J.
Re: [HACKERS] pg9.6 segfault using simple query (related to use fk for join estimates)
... BTW, another thought occurred to me yesterday: it seems like the existing code hasn't thought through its behavior for multiple foreign keys very carefully. That is, suppose we have both "A.J references B.K" and "A.X references B.Y", as separate FKs not a single multicolumn FK. The current code goes to some lengths to decide that one of these is better than the other and then ignore the other. Why? Seems to me that in such a case you want to behave more nearly as you would for a multicolumn FK, that is discard all the join quals matched to either FK in favor of a single selectivity estimate based on the number of rows in the referenced table. Discarding only the A.J = B.K clause and then multiplying by the independent selectivity of A.X = B.Y is surely just as wrong as what we've historically done for multicolumn FKs. (Correcting for nulls would take a bit of thought, but I wouldn't be surprised if it ends up being the same as for the multicolumn-FK case, at least to within the precision we can hope to get with the available stats for nulls.) 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] Changed SRF in targetlist handling
On 06/06/16 18:30, David G. Johnston wrote: > To clarify, the present behavior is basically a combination of both of > Robert's results. > > If the SRFs return the same number of rows the first (zippered) result > is returned without an NULL padding. > > If the SRFs return a different number of rows the LCM behavior kicks in > and you get Robert's second result. No. > SELECT generate_series(1, 4), generate_series(1, 4) ORDER BY 1, 2; > is the same as > SELECT * FROM ROWS FROM ( generate_series(1, 4), generate_series(1, 4) ); > > BUT > > SELECT generate_series(1, 3), generate_series(1, 4) ORDER BY 1, 2; > is the same as > SELECT * FROM ROWS FROM generate_series(1, 3) a, LATERAL ROWS FROM > generate_series(1, 4) b; What would you do with: SELECT generate_series(1, 3), generate_series(1, 6); ? > Tom's 2.5 proposal basically says we make the former equivalence succeed > and have the later one fail. > > The rewrite would be unaware of the cardinality of the SRF and so it > cannot conditionally rewrite the query. One of the two must be chosen > and the incompatible behavior turned into an error. -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- 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] Changed SRF in targetlist handling
"David G. Johnston" writes: > If the SRFs return a different number of rows the LCM behavior kicks in and > you get Robert's second result. Only if the periods of the SRFs are relatively prime. That is, neither of his examples demonstrate the full weirdness of the current behavior; for that, you need periods that are multiples of each other. For instance: SELECT generate_series(1, 2), generate_series(1, 4); generate_series | generate_series -+- 1 | 1 2 | 2 1 | 3 2 | 4 (4 rows) That doesn't comport with any behavior available from LATERAL. 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] pg9.6 segfault using simple query (related to use fk for join estimates)
On 06/06/2016 06:15 PM, Tom Lane wrote: Tomas Vondra writes: On 06/04/2016 08:15 PM, Tom Lane wrote: * Make RelationGetFKeyList cache a list of ForeignKeyOptInfo structs, not just constraint OIDs. It's insane that the relcache scans pg_constraint to collect those OIDs and then the planner re-reads all those same rows on every planning cycle. That seems like a fairly straightforward change, and I'm not opposed to doing that. However RelationGetFKeyList is basically a modified copy of RelationGetIndexList, so it shares the same general behavior, including caching of OIDs and then constructing IndexOptInfo objects on each get_relation_info() call. Why is it 'insane' for foreign keys but not for indexes? Or what am I missing? I would not be in favor of migrating knowledge of IndexOptInfo into the relcache; it's too planner-specific. Also, it mostly copies info from the index's relcache entry, not the parent relation's (which for one thing would imply locking hazards if we tried to cache that info in the parent rel). But for foreign keys, we can cache an image of certain well-defined fields of certain well-defined rows of pg_constraint, and that seems like a reasonably arm's-length definition of a responsibility to give the relcache, especially when it has to visit all and only those same rows to construct what it's caching now. would be okay if you checked after identifying a matching eclass member that it belonged to the FK's referenced table, but AFAICS there is no such check anywhere. Perhaps I'm missing something, but I thought this is checked by these conditions in quals_match_foreign_key(): 1) with ECs (line 3990) if (foreignrel->relid == var->varno && fkinfo->confkeys[i] == var->varattno) foundvarmask |= 1; This checks that you found a joinclause mentioning foreignrel. But foreignrel need have nothing to do with the foreign key; it could be any table in the query. That comparison of confkeys[] and varattno is thus checking for column-number equality of two columns that might be from different relations. That is, if we have an FK "A.X references B.Y", and the query contains "A.X = C.Z", this code could match the FK to that clause if Y and Z happen to have the same data types and column numbers. I don't follow. How could it have 'nothing to do with the foreign key'? We're explicitly checking both varno and varattno on both sides of the foreign key, and we only consider the column is considered matched if both the checks pass. I've tried to come up with an example triggering the issue, but unsuccessfully ... regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, 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] pg9.6 segfault using simple query (related to use fk for join estimates)
On 06/06/2016 06:34 PM, Tom Lane wrote: ... BTW, another thought occurred to me yesterday: it seems like the existing code hasn't thought through its behavior for multiple foreign keys very carefully. That is, suppose we have both "A.J references B.K" and "A.X references B.Y", as separate FKs not a single multicolumn FK. The current code goes to some lengths to decide that one of these is better than the other and then ignore the other. Why? Seems to me that in such a case you want to behave more nearly as you would for a multicolumn FK, that is discard all the join quals matched to either FK in favor of a single selectivity estimate based on the number of rows in the referenced table. Discarding only the A.J = B.K clause and then multiplying by the independent selectivity of A.X = B.Y is surely just as wrong as what we've historically done for multicolumn FKs. (Correcting for nulls would take a bit of thought, but I wouldn't be surprised if it ends up being the same as for the multicolumn-FK case, at least to within the precision we can hope to get with the available stats for nulls.) Yes, that can be improved. The plan was to improve the common case first, and then look at the more complicated cases. It might seem like a hand-waving but I'd bet 99% tables are joined on a single FK, so this seems like a reasonable approach. When it comes to improving multiple (multi-column) foreign keys, I think it may get way more complicated that it might seem. What if the foreign keys overlap, for example? Or what if the keys go in opposite directions (cycle). And so on ... regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, 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] pg9.6 segfault using simple query (related to use fk for join estimates)
Tomas Vondra writes: > On 06/06/2016 06:15 PM, Tom Lane wrote: >> This checks that you found a joinclause mentioning foreignrel. But >> foreignrel need have nothing to do with the foreign key; it could be any >> table in the query. > I don't follow. How could it have 'nothing to do with the foreign key'? Precisely that: clauselist_join_selectivity iterates over every table in the join as a potential foreignrel, and you explicitly refuse to check that that table has anything to do with the foreign key's referenced side. Here's an example: drop table if exists t1, t2, t3; create table t1(f1 int, f2 int, primary key(f1,f2)); insert into t1 select x,x from generate_series(1,10) x; create table t2 (f1 int, f2 int, foreign key(f1,f2) references t1); insert into t2 select (x+10)/10,(x+10)/10 from generate_series(1,10) x; create table t3(f1 int, f2 int); insert into t3 select (x+10)/10,(x+10)/10 from generate_series(1,10) x; analyze t1; analyze t2; analyze t3; explain select * from t1 join t2 on t1.f1=t2.f1 and t1.f2=t2.f2; explain select * from t3 join t2 on t3.f1=t2.f1 and t3.f2=t2.f2; 9.5 estimates the first query as producing 1 row, the second as producing 100 rows. Both of those estimates suck, of course, but it's what you'd expect from treating the joinclauses as uncorrelated. HEAD estimates them both at 10 rows, which is correct for the first query but a pure flight of fancy for the second query. Tracing through this shows that it's accepting t2's FK as a reason to make the estimate, even though t1 doesn't even appear in that query! If we made the modifications previously discussed to throw away FKs that don't connect two tables mentioned in the query, this particular example would stop failing. But it would only take a three-table query involving t1, t2, and t3 to bring the bug back to life, whether or not the join conditions actually match the FK. 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] pg9.6 segfault using simple query (related to use fk for join estimates)
Tomas Vondra writes: > When it comes to improving multiple (multi-column) foreign keys, I think > it may get way more complicated that it might seem. What if the foreign > keys overlap, for example? Or what if the keys go in opposite directions > (cycle). And so on ... I think you can group all FKs referencing the same table and discard all their matched join clauses in favor of a single 1/N estimate (and when I say "discard", that means you don't match those clauses against later FKs, which should take care of the reciprocal-FK issue). This is clearly correct if no nulls are involved. We need to make some estimate of how much to de-rate that figure for nulls, but I don't see that it's any harder than what's already required for a single multicol FK. 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] Changed SRF in targetlist handling
On Mon, Jun 6, 2016 at 11:50 AM, Tom Lane wrote: > Robert Haas writes: >> On Mon, May 23, 2016 at 4:15 PM, Tom Lane wrote: >>> 2. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...). This would >>> have the same behavior as before if the SRFs all return the same number >>> of rows, and otherwise would behave differently. > >> I thought the idea was to rewrite it as LATERAL ROWS FROM (srf1()), >> LATERAL ROWS FROM (srf2()), ... > > No, because then you get the cross-product of multiple SRFs, not the > run-in-lockstep behavior. Oh. I assumed that was the expected behavior. But, ah, what do I know? >> The rewrite you propose here seems to NULL-pad rows after the first >> SRF is exhausted: > > Yes. That's why I said it's not compatible if the SRFs don't all return > the same number of rows. It seems like a reasonable definition to me > though, certainly much more reasonable than the current run-until-LCM > behavior. I can't argue with that. -- 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] Reviewing freeze map code
On Mon, Jun 6, 2016 at 11:44 AM, Andres Freund wrote: >> In terms of diagnostic tools, you can get the VM bits and >> page-level bits using the pg_visibility extension; I wrote it >> precisely because of concerns like the ones you raise here. If you >> want to cross-check the page-level bits against the tuple-level bits, >> you can do that with the pageinspect extension. And if you do those >> things, you can actually find out whether stuff is broken. > > That's WAY out ouf reach of any "normal users". Adding a vacuum option > is doable, writing complex queries is not. Why would they have to write the complex query? Wouldn't they just need to run that we wrote for them? I mean, I'm not 100% dead set against this option you want, but in all honestly, I would never, ever tell anyone to use it. Unleashing VACUUM on possibly-damaged data is just asking it to decide to prune away tuples you don't want gone. I would try very hard to come up with something to give that user that was only going to *read* the possibly-damaged data with as little chance of modifying or erasing it as possible. -- 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] pg9.6 segfault using simple query (related to use fk for join estimates)
On 06/06/2016 07:40 PM, Tom Lane wrote: Tomas Vondra writes: On 06/06/2016 06:15 PM, Tom Lane wrote: This checks that you found a joinclause mentioning foreignrel. But foreignrel need have nothing to do with the foreign key; it could be any table in the query. I don't follow. How could it have 'nothing to do with the foreign key'? Precisely that: clauselist_join_selectivity iterates over every table in the join as a potential foreignrel, and you explicitly refuse to check that that table has anything to do with the foreign key's referenced side. Here's an example: drop table if exists t1, t2, t3; create table t1(f1 int, f2 int, primary key(f1,f2)); insert into t1 select x,x from generate_series(1,10) x; create table t2 (f1 int, f2 int, foreign key(f1,f2) references t1); insert into t2 select (x+10)/10,(x+10)/10 from generate_series(1,10) x; create table t3(f1 int, f2 int); insert into t3 select (x+10)/10,(x+10)/10 from generate_series(1,10) x; analyze t1; analyze t2; analyze t3; explain select * from t1 join t2 on t1.f1=t2.f1 and t1.f2=t2.f2; explain select * from t3 join t2 on t3.f1=t2.f1 and t3.f2=t2.f2; 9.5 estimates the first query as producing 1 row, the second as producing 100 rows. Both of those estimates suck, of course, but it's what you'd expect from treating the joinclauses as uncorrelated. HEAD estimates them both at 10 rows, which is correct for the first query but a pure flight of fancy for the second query. Tracing through this shows that it's accepting t2's FK as a reason to make the estimate, even though t1 doesn't even appear in that query! D'oh! Clearly we need to check confrelid somewhere, not just varno/varattno. I think this should do the trick rte = planner_rt_fetch(var->varno, root); if (foreignrel->relid == var->varno && fkinfo->confrelid == rte->relid && fkinfo->confkeys[i] == var->varattno) foundvarmask |= 1; It seems to resolve the the issue (the estimate is now just 100), but I'm not going to claim it's 100% correct. In any case, thanks for point this out. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, 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] hstore: add hstore_length function
On Fri, Jun 3, 2016 at 7:58 AM, Korbin Hoffman wrote: > > Hi there- > > I've attached a small patch exposing HS_COUNT to the user as > "hstore_length(hstore)". Documentation, upgrade sql files, and a few > tests are also included. > > Almost every hstore function calls HS_COUNT, but I couldn't determine > if there was a reason this exposure didn't already exist. > > Without this function, I've been converting an hstore into an array > and then counting it - a more expensive operation (~30-40% slower than > SELECTing the hstore itself in a few of my tests). > > I will add this thread and patch to the next Commitfest. > Something goes wrong when applying against master: $ git apply ~/Downloads/hstore_length-v1.patch error: contrib/hstore/Makefile: already exists in working directory error: contrib/hstore/expected/hstore.out: already exists in working directory error: contrib/hstore/hstore--1.3.sql: already exists in working directory error: contrib/hstore/hstore.control: already exists in working directory error: contrib/hstore/hstore_op.c: already exists in working directory error: contrib/hstore/sql/hstore.sql: already exists in working directory error: doc/src/sgml/hstore.sgml: already exists in working directory Anyway I have some comments: 1) I don't see any reason to add this sort of thing if you're adding a new function + HSTORE_POLLUTE(hstore_length, length); 2) Shouldn't this declaration use 'uint32' instead of 'int' ?? + int count = HS_COUNT(hs); + + PG_RETURN_INT32(count); maybe + uint32 count = HS_COUNT(hs); + + PG_RETURN_UINT32(count); 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
Re: [HACKERS] Reviewing freeze map code
* Robert Haas (robertmh...@gmail.com) wrote: > On Mon, Jun 6, 2016 at 11:44 AM, Andres Freund wrote: > >> In terms of diagnostic tools, you can get the VM bits and > >> page-level bits using the pg_visibility extension; I wrote it > >> precisely because of concerns like the ones you raise here. If you > >> want to cross-check the page-level bits against the tuple-level bits, > >> you can do that with the pageinspect extension. And if you do those > >> things, you can actually find out whether stuff is broken. > > > > That's WAY out ouf reach of any "normal users". Adding a vacuum option > > is doable, writing complex queries is not. > > Why would they have to write the complex query? Wouldn't they just > need to run that we wrote for them? > > I mean, I'm not 100% dead set against this option you want, but in all > honestly, I would never, ever tell anyone to use it. Unleashing > VACUUM on possibly-damaged data is just asking it to decide to prune > away tuples you don't want gone. I would try very hard to come up > with something to give that user that was only going to *read* the > possibly-damaged data with as little chance of modifying or erasing it > as possible. I certainly agree with this. We need a read-only utility which checks that the system is in a correct and valid state. There are a few of those which have been built for different pieces, I believe, and we really should have one for the visibility map, but I don't think it makes sense to imply in any way that VACUUM can or should be used for that. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Changed SRF in targetlist handling
Robert Haas wrote: > On Mon, Jun 6, 2016 at 11:50 AM, Tom Lane wrote: > > Robert Haas writes: > >> On Mon, May 23, 2016 at 4:15 PM, Tom Lane wrote: > >>> 2. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...). This would > >>> have the same behavior as before if the SRFs all return the same number > >>> of rows, and otherwise would behave differently. > > > >> I thought the idea was to rewrite it as LATERAL ROWS FROM (srf1()), > >> LATERAL ROWS FROM (srf2()), ... > > > > No, because then you get the cross-product of multiple SRFs, not the > > run-in-lockstep behavior. > > Oh. I assumed that was the expected behavior. But, ah, what do I know? Lots, I assume -- but in this case, probably next to nothing, just like most of us, because what sane person or application would be really relying on the wacko historical behavior, in order to generate some collective knowledge? However, I think that it is possible that someone, somewhere has two SRFs-in-targetlist that return the same number of rows and that the current implementation works fine for them; if we redefine it to work differently, we would break their application silently, which seems a worse problem than breaking it noisily while providing an easy way forward (which is to move SRFs to the FROM list) My vote is to raise an error in the case of more than one SRF in targetlist. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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] Reviewing freeze map code
On 2016-06-06 14:24:14 -0400, Stephen Frost wrote: > * Robert Haas (robertmh...@gmail.com) wrote: > > On Mon, Jun 6, 2016 at 11:44 AM, Andres Freund wrote: > > >> In terms of diagnostic tools, you can get the VM bits and > > >> page-level bits using the pg_visibility extension; I wrote it > > >> precisely because of concerns like the ones you raise here. If you > > >> want to cross-check the page-level bits against the tuple-level bits, > > >> you can do that with the pageinspect extension. And if you do those > > >> things, you can actually find out whether stuff is broken. > > > > > > That's WAY out ouf reach of any "normal users". Adding a vacuum option > > > is doable, writing complex queries is not. > > > > Why would they have to write the complex query? Wouldn't they just > > need to run that we wrote for them? Then write that query. Verify that that query performs halfway reasonably fast. Document that it should be run against databases after subjecting them to tests. That'd address my concern as well. > > I mean, I'm not 100% dead set against this option you want, but in all > > honestly, I would never, ever tell anyone to use it. Unleashing > > VACUUM on possibly-damaged data is just asking it to decide to prune > > away tuples you don't want gone. I would try very hard to come up > > with something to give that user that was only going to *read* the > > possibly-damaged data with as little chance of modifying or erasing it > > as possible. I'm more concerned about actually being able to verify that the freeze logic does actually something meaningful, in situation where we'd *NOT* expect any problems. If we're not trusting vacuum in that situation, well ... > I certainly agree with this. > > We need a read-only utility which checks that the system is in a correct > and valid state. There are a few of those which have been built for > different pieces, I believe, and we really should have one for the > visibility map, but I don't think it makes sense to imply in any way > that VACUUM can or should be used for that. Meh. This is vacuum behaviour that *has existed* up to this point. You essentially removed it. Sure, I'm all for adding a verification tool. But that's just pie in the skie at this point. We have a complex, data loss threatening feature, which just about nobody can verify at this point. That's crazy. Greetings, Andres Freund -- 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] [BUGS] Routine analyze of single column prevents standard autoanalyze from running at all
[ redirecting to -hackers ] Tomasz Ostrowski writes: > I'm routinely bulk inserting data to a PostgreSQL table and then > analyzing a single column of the table, because it contains data which > significantly changes histogram of this column values - for example > something like adding rows with "todo=true" column, when all rows before > bulk insert have "todo=false". > But I've noticed that a standard automatic analyze, which should work in > background, never runs. I've noticed that this fast analyze of one > column resets pg_stat_user_tables(n_mod_since_analyze) counter. > I suppose this is a bug - an analyze, which does not analyze all > columns, should not reset pg_stat_user_tables(n_mod_since_analyze). What > do you think? I'm inclined to think that this is a reasonable complaint. A usage pattern like that probably hasn't come up before; but now that it has, it's clear it shouldn't block auto-analyze from happening. A cheap-and-dirty solution would be to not send a PgStat_MsgAnalyze message at all, but I think that's probably undesirable: typically it would be a good thing to accept the new n_live_tuples and n_dead_tuples estimates. What we could do instead is add a bool flag to PgStat_MsgAnalyze saying whether or not to reset changes_since_analyze. It would be safe enough to back-patch such a change, because the stats collector messages are private to the backend (in fact, really private to pgstat.c). One question here is whether there is any connection between changes_since_analyze and the tuple-count estimates that would make it improper to update the latter and not the former. I can't really see one; in fact, changes_since_analyze is pretty squishy anyway because changes made by an ANALYZE's own transaction are already accounted for in the new pg_statistic entries but will be added to changes_since_analyze at commit despite that. So I'd go ahead and update all the fields other than changes_since_analyze in this case. Another interesting consideration is that if the table's columns have different stats targets, a selective-column ANALYZE might possibly sample fewer rows than an all-columns ANALYZE would. This might mean that our new tuple-count estimates are less accurate than autoanalyze would get. So you could possibly argue that we shouldn't update the tuple-count estimates after all. But I don't particularly believe that, because it disregards the fact that the new estimates are, well, new. Even if they have more statistical risk than autoanalyze would have, they could well be better just by virtue of having seen whatever bulk updates might have happened since the last autoanalyze. So my inclination is to disregard this fine point. (Though it's interesting to ask whether Tomasz's use case includes a lower-than-default stats target for his very volatile column ...) Also, I'd be a bit inclined to disable the counter reset whenever a column list is specified, disregarding the corner case where a list is given but it includes all the table's analyzable columns. It doesn't really seem worth the effort to account for that case specially (especially after you consider that index expressions should count as analyzable columns). Thoughts? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changed SRF in targetlist handling
On Mon, Jun 6, 2016 at 2:31 PM, Alvaro Herrera wrote: > Robert Haas wrote: > > On Mon, Jun 6, 2016 at 11:50 AM, Tom Lane wrote: > > > Robert Haas writes: > > >> On Mon, May 23, 2016 at 4:15 PM, Tom Lane wrote: > > >>> 2. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...). This would > > >>> have the same behavior as before if the SRFs all return the same > number > > >>> of rows, and otherwise would behave differently. > > > > > >> I thought the idea was to rewrite it as LATERAL ROWS FROM (srf1()), > > >> LATERAL ROWS FROM (srf2()), ... > > > > > > No, because then you get the cross-product of multiple SRFs, not the > > > run-in-lockstep behavior. > > > > Oh. I assumed that was the expected behavior. But, ah, what do I know? > > Lots, I assume -- but in this case, probably next to nothing, just like > most of us, because what sane person or application would be really > relying on the wacko historical behavior, in order to generate some > collective knowledge? However, I think that it is possible that > someone, somewhere has two SRFs-in-targetlist that return the same > number of rows and that the current implementation works fine for them; > if we redefine it to work differently, we would break their application > silently, which seems a worse problem than breaking it noisily while > providing an easy way forward (which is to move SRFs to the FROM list) > > My vote is to raise an error in the case of more than one SRF in > targetlist. > As long as someone is willing to put in the effort we can make a subset of these multiple-SRFs-in-targetlist queries work without any change in the tabular output, though the processing mechanism might change. Your vote is essentially #1 up-thread which seems the most draconian. Assuming a viable option 2.5 or 3 solution is presented would you vote against it being committed? If so I'd like to understand why. I see #1 as basically OK only if their are technical barriers we cannot overcome - including performance. Link to the definition of the various options Tom proposed: https://www.postgresql.org/message-id/21076.1464034513%40sss.pgh.pa.us David J.
Re: [HACKERS] [sqlsmith] Failed assertions on parallel worker shutdown
On Sat, Jun 4, 2016 at 2:57 AM, Amit Kapila wrote: > In the above change, you are first adding bytes_written and then doing the > SHM_MQ_DETACHED check, whereas other place the check is done first which > seems to be right. I think it doesn't matter either way, but it is better > to be consistent. Also isn't it better to set mqh_length_word_complete as > false as next time this API is called, it should first try to write length > into buffer. OK, committed after (I hope) fixing those issues. -- 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] Changed SRF in targetlist handling
Alvaro Herrera writes: > Robert Haas wrote: >> On Mon, Jun 6, 2016 at 11:50 AM, Tom Lane wrote: >>> No, because then you get the cross-product of multiple SRFs, not the >>> run-in-lockstep behavior. >> Oh. I assumed that was the expected behavior. But, ah, what do I know? > Lots, I assume -- but in this case, probably next to nothing, just like > most of us, because what sane person or application would be really > relying on the wacko historical behavior, in order to generate some > collective knowledge? However, I think that it is possible that > someone, somewhere has two SRFs-in-targetlist that return the same > number of rows and that the current implementation works fine for them; Yes. Run-in-lockstep is an extremely useful behavior, so much so that we made a LATERAL variant for it. I do not see a reason to break such cases in the targetlist. > My vote is to raise an error in the case of more than one SRF in targetlist. Note that that risks breaking cases that the user does not think are "more than one SRF". Consider this example using a regression-test table: regression=# create function foo() returns setof int8_tbl as regression-# 'select * from int8_tbl' language sql; CREATE FUNCTION regression=# select foo(); foo -- (123,456) (123,4567890123456789) (4567890123456789,123) (4567890123456789,4567890123456789) (4567890123456789,-4567890123456789) (5 rows) regression=# explain verbose select foo(); QUERY PLAN -- Result (cost=0.00..5.25 rows=1000 width=32) Output: foo() (2 rows) regression=# select (foo()).*; q1|q2 --+--- 123 | 456 123 | 4567890123456789 4567890123456789 | 123 4567890123456789 | 4567890123456789 4567890123456789 | -4567890123456789 (5 rows) regression=# explain verbose select (foo()).*; QUERY PLAN -- Result (cost=0.00..5.50 rows=1000 width=16) Output: (foo()).q1, (foo()).q2 (2 rows) The reason we can get away with this simplistic treatment of composite-returning SRFs is precisely the run-in-lockstep behavior. Otherwise the second query would have returned 25 rows. Now, if we decide to try to rewrite tlist SRFs as LATERAL, it would likely behoove us to do that rewrite before expanding * not after, so that we can eliminate the multiple evaluation of foo() that happens currently. (That makes it a parser problem not a planner problem.) And maybe we should rewrite non-SRF composite-returning functions this way too, because people have definitely complained about the extra evaluations in that context. But my point here is that lockstep evaluation does have practical use when the SRFs are iterating over matching collections of generated rows. And that seems like a pretty common use-case. 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] Changed SRF in targetlist handling
On Mon, Jun 6, 2016 at 2:53 PM, Tom Lane wrote: > Now, if we decide to try to rewrite tlist SRFs as LATERAL, it would likely > behoove us to do that rewrite before expanding * not after, so that we can > eliminate the multiple evaluation of foo() that happens currently. (That > makes it a parser problem not a planner problem.) And maybe we should > rewrite non-SRF composite-returning functions this way too, because people > have definitely complained about the extra evaluations in that context. > But my point here is that lockstep evaluation does have practical use > when the SRFs are iterating over matching collections of generated rows. > And that seems like a pretty common use-case. Yeah, OK. I'm not terribly opposed to going that way. I think the current behavior sucks badly enough - both because the semantics are bizarre and because it complicates the whole executor for a niche feature - that it's worth taking a backward compatibility hit to change it. I guess I'd prefer #2 to #2.5, #2.5 to #3, and #3 to #1. I really don't like #1 much - I think I'd almost rather do nothing. -- 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] Reviewing freeze map code
On Mon, Jun 6, 2016 at 2:35 PM, Andres Freund wrote: >> > Why would they have to write the complex query? Wouldn't they just >> > need to run that we wrote for them? > > Then write that query. Verify that that query performs halfway > reasonably fast. Document that it should be run against databases after > subjecting them to tests. That'd address my concern as well. You know, I am starting to lose a teeny bit of patience here. I do appreciate you reviewing this code, very much, and genuinely, and it would be great if more people wanted to review it. But this kind of reads like you think that I'm being a jerk, which I'm trying pretty hard not to be, and like you have the right to tell assign me arbitrary work, which I think you don't. If you want to have a reasonable conversation about what the options are for making this better, great. If you want to me to do some work to help improve things on a patch I committed, that is 100% fair. But I don't know what I did to earn this response which, to me, reads as rather demanding and rather exasperated. -- 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] [sqlsmith] Failed assertions on parallel worker shutdown
On Fri, Jun 3, 2016 at 11:13 PM, Robert Haas wrote: >> I think the workers should stop processing tuples after the tuple queues got >> detached. This will not only handle above situation gracefully, but will >> allow to speed up the queries where Limit clause is present on top of Gather >> node. Patch for the same is attached with mail (this was part of original >> parallel seq scan patch, but was not applied and the reason as far as I >> remember was we thought such an optimization might not be required for >> initial version). > > This is very likely a good idea, but... > >> Another approach to fix this issue could be to reset mqh_partial_bytes and >> mqh_length_word_complete in shm_mq_sendv in case of SHM_MQ_DETACHED. These >> are currently reset only incase of success. > > ...I think we should do this too Patches for both of these things are now committed. Barring problems with those commits, I believe this resolves this open item. -- 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] Changed SRF in targetlist handling
Robert Haas writes: > ... I guess I'd prefer #2 to #2.5, #2.5 to #3, and #3 to #1. > I really don't like #1 much - I think I'd almost rather do nothing. FWIW, that's about my evaluation of the alternatives as well. I fear that #1 would get a lot of pushback. If we think that something like "LATERAL ROWS FROM STRICT" is worth having on its own merits, then doing #2.5 seems worthwhile to me, but otherwise I'm just as happy with #2. David J. seems to feel that throwing an error (as in #2.5) rather than silently behaving incompatibly (as in #2) is important, but I'm not convinced. In a green field I think we'd prefer #2 over #2.5, so I'd rather go that direction. 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] [BUGS] Routine analyze of single column prevents standard autoanalyze from running at all
I wrote:
> Tomasz Ostrowski writes:
>> I suppose this is a bug - an analyze, which does not analyze all
>> columns, should not reset pg_stat_user_tables(n_mod_since_analyze). What
>> do you think?
> I'm inclined to think that this is a reasonable complaint. A usage
> pattern like that probably hasn't come up before; but now that it has,
> it's clear it shouldn't block auto-analyze from happening.
Attached is a draft patch for this, which I propose to apply and
back-patch. Any objections?
regards, tom lane
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index cf8c816..97059e5 100644
*** a/src/backend/commands/analyze.c
--- b/src/backend/commands/analyze.c
*** do_analyze_rel(Relation onerel, int opti
*** 611,620
/*
* Report ANALYZE to the stats collector, too. However, if doing
* inherited stats we shouldn't report, because the stats collector only
! * tracks per-table stats.
*/
if (!inh)
! pgstat_report_analyze(onerel, totalrows, totaldeadrows);
/* If this isn't part of VACUUM ANALYZE, let index AMs do cleanup */
if (!(options & VACOPT_VACUUM))
--- 611,623
/*
* Report ANALYZE to the stats collector, too. However, if doing
* inherited stats we shouldn't report, because the stats collector only
! * tracks per-table stats. Reset the changes_since_analyze counter only
! * if we analyzed all columns; otherwise, there is still work for
! * auto-analyze to do.
*/
if (!inh)
! pgstat_report_analyze(onerel, totalrows, totaldeadrows,
! (va_cols == NIL));
/* If this isn't part of VACUUM ANALYZE, let index AMs do cleanup */
if (!(options & VACOPT_VACUUM))
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 44237f9..439fc06 100644
*** a/src/backend/postmaster/pgstat.c
--- b/src/backend/postmaster/pgstat.c
*** pgstat_report_vacuum(Oid tableoid, bool
*** 1340,1350
* pgstat_report_analyze() -
*
* Tell the collector about the table we just analyzed.
*
*/
void
pgstat_report_analyze(Relation rel,
! PgStat_Counter livetuples, PgStat_Counter deadtuples)
{
PgStat_MsgAnalyze msg;
--- 1340,1354
* pgstat_report_analyze() -
*
* Tell the collector about the table we just analyzed.
+ *
+ * Caller must provide new live- and dead-tuples estimates, as well as a
+ * flag indicating whether to reset the changes_since_analyze counter.
*
*/
void
pgstat_report_analyze(Relation rel,
! PgStat_Counter livetuples, PgStat_Counter deadtuples,
! bool resetcounter)
{
PgStat_MsgAnalyze msg;
*** pgstat_report_analyze(Relation rel,
*** 1381,1386
--- 1385,1391
msg.m_databaseid = rel->rd_rel->relisshared ? InvalidOid : MyDatabaseId;
msg.m_tableoid = RelationGetRelid(rel);
msg.m_autovacuum = IsAutoVacuumWorkerProcess();
+ msg.m_resetcounter = resetcounter;
msg.m_analyzetime = GetCurrentTimestamp();
msg.m_live_tuples = livetuples;
msg.m_dead_tuples = deadtuples;
*** pgstat_recv_analyze(PgStat_MsgAnalyze *m
*** 5263,5272
tabentry->n_dead_tuples = msg->m_dead_tuples;
/*
! * We reset changes_since_analyze to zero, forgetting any changes that
! * occurred while the ANALYZE was in progress.
*/
! tabentry->changes_since_analyze = 0;
if (msg->m_autovacuum)
{
--- 5268,5278
tabentry->n_dead_tuples = msg->m_dead_tuples;
/*
! * If commanded, reset changes_since_analyze to zero, forgetting any
! * changes that occurred while the ANALYZE was in progress.
*/
! if (msg->m_resetcounter)
! tabentry->changes_since_analyze = 0;
if (msg->m_autovacuum)
{
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index a078b61..19c8387 100644
*** a/src/include/pgstat.h
--- b/src/include/pgstat.h
*** typedef struct PgStat_MsgAnalyze
*** 383,388
--- 383,389
Oid m_databaseid;
Oid m_tableoid;
bool m_autovacuum;
+ bool m_resetcounter;
TimestampTz m_analyzetime;
PgStat_Counter m_live_tuples;
PgStat_Counter m_dead_tuples;
*** extern void pgstat_report_autovac(Oid db
*** 970,976
extern void pgstat_report_vacuum(Oid tableoid, bool shared,
PgStat_Counter livetuples, PgStat_Counter deadtuples);
extern void pgstat_report_analyze(Relation rel,
! PgStat_Counter livetuples, PgStat_Counter deadtuples);
extern void pgstat_report_recovery_conflict(int reason);
extern void pgstat_report_deadlock(void);
--- 971,978
extern void pgstat_report_vacuum(Oid tableoid, bool shared,
PgStat_Counter livetuples, PgStat_Counter deadtuples);
extern void pgstat_report_analyze(Relation rel,
! PgStat_Counter livetuples, PgStat_Counter deadtuples,
! bool resetcounter);
extern void pgstat_report_recovery_conflict(int reason);
extern void pgstat_report_deadlock(void);
-
Re: [HACKERS] Reviewing freeze map code
Hi, On 2016-06-06 15:16:10 -0400, Robert Haas wrote: > On Mon, Jun 6, 2016 at 2:35 PM, Andres Freund wrote: > >> > Why would they have to write the complex query? Wouldn't they just > >> > need to run that we wrote for them? > > > > Then write that query. Verify that that query performs halfway > > reasonably fast. Document that it should be run against databases after > > subjecting them to tests. That'd address my concern as well. > > You know, I am starting to lose a teeny bit of patience here. Same here. > I do appreciate you reviewing this code, very much, and genuinely, and > it would be great if more people wanted to review it. > But this kind of reads like you think that I'm being a jerk, which I'm > trying pretty hard not to be I don't think you're a jerk. But I am loosing a good bit of my patience here. I've posted these issues a month ago, and for a long while the only thing that happened was bikeshedding about the name of something that wasn't even decided to happen yet (obviously said bikeshedding isn't your fault). > and like you have the right to tell assign me arbitrary work, which I > think you don't. It's not like adding a parameter for this would be a lot of work, there's even a patch out there. I'm getting impatient because I feel the issue of this critical feature not being testable is getting ignored and/or played down. And then sidetracked into a general "let's add a database consistency checker" type discussion. Which we need, but won't get in 9.6. If you say: "I agree with the feature in principle, but I don't want to spend time to review/commit it." - ok, that's fair enough. But at the moment that isn't what I'm reading between the lines. > If you want to have a > reasonable conversation about what the options are for making this > better, great. Yes, I want that. > If you want to me to do some work to help improve things on a patch I > committed, that is 100% fair. But I don't know what I did to earn > this response which, to me, reads as rather demanding and rather > exasperated. I don't think it's absurd to make some demands on the committer of a impact-heavy feature, about at least finding a realistic path towards the new feature being realistically testable. This is a scary (but *REALLY IMPORTANT*) patch, and I don't understand why it's ok that we can't push a it through a couple wraparounds under high concurrency, and easily verify that the freeze map is in sync with the actual data. And yes, I *am* exasperated, that I'm the only one that appears to be scared by the lack of that capability. I think the feature is in a *lot* better shape than multixacts, but it certainly has the potential to do even more damage in ways that'll essentially be unrecoverable. Andres -- 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] Reviewing freeze map code
Andres, all, * Andres Freund (and...@anarazel.de) wrote: > On 2016-06-06 15:16:10 -0400, Robert Haas wrote: > > On Mon, Jun 6, 2016 at 2:35 PM, Andres Freund wrote: > > and like you have the right to tell assign me arbitrary work, which I > > think you don't. > > It's not like adding a parameter for this would be a lot of work, > there's even a patch out there. I'm getting impatient because I feel > the issue of this critical feature not being testable is getting ignored > and/or played down. And then sidetracked into a general "let's add a > database consistency checker" type discussion. Which we need, but won't > get in 9.6. To be clear, I was pointing out that we've had similar types of consistency checkers implemented for other big features (eg: Heikki's work on checking that WAL works) and that it'd be good to have one here also. That could be as simple as a query with the right things installed, or it might be an independent tool, but not having any way to check isn't good. That said, trying to make VACUUM do that doesn't make sense to me either. Perhaps that's not an option due to the lateness of the hour or the lack of manpower behind it, but that doesn't seem to be what has been said so far. > > If you want to me to do some work to help improve things on a patch I > > committed, that is 100% fair. But I don't know what I did to earn > > this response which, to me, reads as rather demanding and rather > > exasperated. > > I don't think it's absurd to make some demands on the committer of a > impact-heavy feature, about at least finding a realistic path towards > the new feature being realistically testable. This is a scary (but > *REALLY IMPORTANT*) patch, and I don't understand why it's ok that we > can't push a it through a couple wraparounds under high concurrency, and > easily verify that the freeze map is in sync with the actual data. > > And yes, I *am* exasperated, that I'm the only one that appears to be > scared by the lack of that capability. I think the feature is in a > *lot* better shape than multixacts, but it certainly has the potential > to do even more damage in ways that'll essentially be unrecoverable. Not having a straightforward way to ensure that it's working properly is certainly concerning to me as well. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [BUGS] Routine analyze of single column prevents standard autoanalyze from running at all
On 06/06/2016 01:38 PM, Tom Lane wrote: > Also, I'd be a bit inclined to disable the counter reset whenever a column > list is specified, disregarding the corner case where a list is given but > it includes all the table's analyzable columns. It doesn't really seem > worth the effort to account for that case specially (especially after > you consider that index expressions should count as analyzable columns). > > Thoughts? +1. Better to err on the side of duplicate analyzes than none at all. Also, I'm not surprised this took so long to discover; I doubt most users are aware that you *can* analyze individual columns. -- -- Josh Berkus Red Hat OSAS (any opinions are my own) -- 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] Reviewing freeze map code
On 2016-06-06 16:18:19 -0400, Stephen Frost wrote:
> That could be as simple as a query with the right things installed, or
> it might be an independent tool, but not having any way to check isn't
> good. That said, trying to make VACUUM do that doesn't make sense to me
> either.
The point is that VACUUM *has* these types of checks. And had so for
many years:
else if (all_visible_according_to_vm && !PageIsAllVisible(page)
&& VM_ALL_VISIBLE(onerel, blkno, &vmbuffer))
{
elog(WARNING, "page is not marked all-visible but
visibility map bit is set in relation \"%s\" page %u",
relname, blkno);
visibilitymap_clear(onerel, blkno, vmbuffer);
}
...
else if (PageIsAllVisible(page) && has_dead_tuples)
{
elog(WARNING, "page containing dead tuples is marked as
all-visible in relation \"%s\" page %u",
relname, blkno);
PageClearAllVisible(page);
MarkBufferDirty(buf);
visibilitymap_clear(onerel, blkno, vmbuffer);
}
the point is that, after the introduction of the freeze bit, there's no
way to reach them anymore (and they're missing a useful extension of
these warnings, but ...); these warnings have caught bugs. I don't
think it'd advocate for the vacuum option otherwise.
Greetings,
Andres Freund
--
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] Reviewing freeze map code
On Mon, Jun 6, 2016 at 4:06 PM, Andres Freund wrote: >> I do appreciate you reviewing this code, very much, and genuinely, and >> it would be great if more people wanted to review it. > >> But this kind of reads like you think that I'm being a jerk, which I'm >> trying pretty hard not to be > > I don't think you're a jerk. But I am loosing a good bit of my patience > here. I've posted these issues a month ago, and for a long while the > only thing that happened was bikeshedding about the name of something > that wasn't even decided to happen yet (obviously said bikeshedding > isn't your fault). No, the bikeshedding is not my fault. As for the timing, you posted your first comments exactly a week before beta1 when I was still busy addressing issues that were reported before you reported yours, and I did not think it was realistic to get them addressed in the time available. If you'd sent them two weeks sooner, I would probably have done so. Now, it's been four weeks since beta1 wrapped, one of which was PGCon. As far as I understand at this point in time, your review identified exactly zero potential data loss bugs. (We thought there was one, but it looks like there isn't.) All of the non-critical defects you identified have now been fixed, apart from the lack of a better testing tool. And since there is ongoing discussion (call it bikeshedding if you want) about what would actually help in that area, I really don't feel like anything very awful is happening here. I really don't understand how you can not weigh in on the original thread leading up to my mid-March commits and say "hey, this needs a better testing tool", and then when you finally get around to reviewing it in May, I'm supposed to drop everything and write one immediately. Why do you get two months from the time of commit to weigh in but I get no time to respond? For my part, I thought I *had* written a testing tool - that's what pg_visibility is and that's what I used to test the feature before committing it. Now, you think that's not good enough, and I respect your opinion, but it's not as if you said this back when this was being committed. Or at least if you did, I don't remember it. >> and like you have the right to tell assign me arbitrary work, which I >> think you don't. > > It's not like adding a parameter for this would be a lot of work, > there's even a patch out there. I'm getting impatient because I feel > the issue of this critical feature not being testable is getting ignored > and/or played down. And then sidetracked into a general "let's add a > database consistency checker" type discussion. Which we need, but won't > get in 9.6. I know there's a patch. Both Tom and I are skeptical about whether it adds value, and I really don't think you've spelled out in as much detail why you think it will help as I have why I think it won't. Initially, I was like "ok, sure, we should have that", but the more I thought about it (another advantage of time passing: you can think about things more) the less convinced I was that it did anything useful. I don't think that's very unreasonable. The importance of the feature is exactly why we *should* think carefully about what is best here and not just do the first thing that pops into our head. > If you say: "I agree with the feature in principle, but I don't want to > spend time to review/commit it." - ok, that's fair enough. But at the > moment that isn't what I'm reading between the lines. No, what I'm saying is "I'm not confident that this feature adds value, and I'm afraid that by adding it we are making ourselves feel better without solving any real problem". I'm also saying "let's try to agree on what problems we need to solve first and then decide on the solutions". >> If you want to have a >> reasonable conversation about what the options are for making this >> better, great. > > Yes, I want that. Great. -- 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] Changed SRF in targetlist handling
On Mon, Jun 6, 2016 at 3:26 PM, Tom Lane wrote: > Robert Haas writes: > > ... I guess I'd prefer #2 to #2.5, #2.5 to #3, and #3 to #1. > > I really don't like #1 much - I think I'd almost rather do nothing. > > FWIW, that's about my evaluation of the alternatives as well. I fear > that #1 would get a lot of pushback. If we think that something like > "LATERAL ROWS FROM STRICT" is worth having on its own merits, then > doing #2.5 seems worthwhile to me, but otherwise I'm just as happy > with #2. David J. seems to feel that throwing an error (as in #2.5) > rather than silently behaving incompatibly (as in #2) is important, > but I'm not convinced. In a green field I think we'd prefer #2 over > #2.5, so I'd rather go that direction. > I suspect the decision to error or not is a one or two line change in whatever form the final patch takes. It seems like approach #2 is acceptable on a theoretical level which implies there is no desire to make the existing LCM behavior available post-patch. Assuming it is simple then everyone will have a chance to make their opinion known on whether the 2.0 or 2.5 variation is preferable for the final commit. If a decision needs to be made sooner due to a design decision I'd hope the author of the patch would make that known so we can bring this to resolution at that point instead. David J.
Re: [HACKERS] Reviewing freeze map code
On Mon, Jun 6, 2016 at 2:35 PM, Andres Freund wrote:
>> > Why would they have to write the complex query? Wouldn't they just
>> > need to run that we wrote for them?
>
> Then write that query. Verify that that query performs halfway
> reasonably fast. Document that it should be run against databases after
> subjecting them to tests. That'd address my concern as well.
Here is a first attempt at such a query. It requires that the
pageinspect and pg_visibility extensions be installed.
SELECT c.oid, v.blkno, array_agg(hpi.lp) AS affect_lps FROM pg_class
c, LATERAL ROWS FROM (pg_visibility(c.oid)) v, LATERAL ROWS FROM
(heap_page_items(get_raw_page(c.oid::regclass::text, blkno::int4)))
hpi WHERE c.relkind IN ('r', 't', 'm') AND v.all_frozen AND
(((hpi.t_infomask & 768) != 768 AND hpi.t_xmin NOT IN (1, 2)) OR
(hpi.t_infomask & 2048) != 2048) GROUP BY 1, 2 ORDER BY 1, 2;
I am not sure this is 100% correct, especially the XMAX-checking part:
is HEAP_XMAX_INVALID guaranteed to be set on a fully-frozen tuple? Is
the method of constructing the first argument to get_raw_page() going
to be robust in all cases?
I'm not sure what the performance will be on a large table, either.
That will have to be checked. And I obviously have not done extensive
stress runs yet. But maybe it's a start. Comments?
--
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] Reviewing freeze map code
On Mon, Jun 6, 2016 at 11:35 AM, Andres Freund wrote: >> We need a read-only utility which checks that the system is in a correct >> and valid state. There are a few of those which have been built for >> different pieces, I believe, and we really should have one for the >> visibility map, but I don't think it makes sense to imply in any way >> that VACUUM can or should be used for that. > > Meh. This is vacuum behaviour that *has existed* up to this point. You > essentially removed it. Sure, I'm all for adding a verification > tool. But that's just pie in the skie at this point. We have a complex, > data loss threatening feature, which just about nobody can verify at > this point. That's crazy. FWIW, I agree with the general sentiment. Building a stress-testing suite would have been a good idea. In general, testability is a design goal that I'd be willing to give up other things for. -- Peter Geoghegan -- 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] Reviewing freeze map code
On Mon, Jun 6, 2016 at 4:27 PM, Andres Freund wrote:
> On 2016-06-06 16:18:19 -0400, Stephen Frost wrote:
>> That could be as simple as a query with the right things installed, or
>> it might be an independent tool, but not having any way to check isn't
>> good. That said, trying to make VACUUM do that doesn't make sense to me
>> either.
>
> The point is that VACUUM *has* these types of checks. And had so for
> many years:
> else if (all_visible_according_to_vm &&
> !PageIsAllVisible(page)
> && VM_ALL_VISIBLE(onerel, blkno, &vmbuffer))
> {
> elog(WARNING, "page is not marked all-visible but
> visibility map bit is set in relation \"%s\" page %u",
> relname, blkno);
> visibilitymap_clear(onerel, blkno, vmbuffer);
> }
> ...
> else if (PageIsAllVisible(page) && has_dead_tuples)
> {
> elog(WARNING, "page containing dead tuples is marked
> as all-visible in relation \"%s\" page %u",
> relname, blkno);
> PageClearAllVisible(page);
> MarkBufferDirty(buf);
> visibilitymap_clear(onerel, blkno, vmbuffer);
> }
>
> the point is that, after the introduction of the freeze bit, there's no
> way to reach them anymore (and they're missing a useful extension of
> these warnings, but ...); these warnings have caught bugs. I don't
> think it'd advocate for the vacuum option otherwise.
So a couple of things:
1. I think it is pretty misleading to say that those checks aren't
reachable any more. It's not like we freeze every page when we mark
it all-visible. In most cases, I think that what will happen is that
the page will be marked all-visible and then, because it is
all-visible, skipped by subsequent vacuums, so that it doesn't get
marked all-frozen until a few hundred million transactions later. Of
course there will be some cases when a page gets marked all-visible
and all-frozen at the same time, but I don't see why we should expect
that to be the norm.
2. With the new pg_visibility extension, you can actually check the
same thing that first warning checks like this:
select * from pg_visibility('t1'::regclass) where all_visible and not
pd_all_visible;
IMHO, that's a substantial improvement over running VACUUM and
checking whether it spits out a WARNING.
The second one, you can't currently trigger for all-frozen pages. The
query I just sent in my other email could perhaps be adapted to that
purpose, but maybe this is a good-enough reason to add VACUUM
(even_frozen_pages).
3. If you think there are analogous checks that I should add for the
frozen case, or that you want to add yourself, please say what they
are specifically. I *did* think about it when I wrote that code and I
didn't see how to make it work. If I had, I would have added them.
The whole point of review here is, hopefully, to illuminate what
should have been done differently - if I'd known how to do it better,
I would have done so. Provide an idea, or better yet, provide a
patch. If you see how to do it, coding it up shouldn't be the hard
part.
Thanks,
--
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] Changed SRF in targetlist handling
On Mon, Jun 6, 2016 at 3:26 PM, Tom Lane wrote: > Robert Haas writes: >> ... I guess I'd prefer #2 to #2.5, #2.5 to #3, and #3 to #1. >> I really don't like #1 much - I think I'd almost rather do nothing. > > FWIW, that's about my evaluation of the alternatives as well. I fear > that #1 would get a lot of pushback. If we think that something like > "LATERAL ROWS FROM STRICT" is worth having on its own merits, then > doing #2.5 seems worthwhile to me, but otherwise I'm just as happy > with #2. David J. seems to feel that throwing an error (as in #2.5) > rather than silently behaving incompatibly (as in #2) is important, > but I'm not convinced. In a green field I think we'd prefer #2 over > #2.5, so I'd rather go that direction. Same here. That behavior is actually potentially quite useful, right? Like, you might want to rely on the NULL-extension thing, if it were documented as behavior you can count on? -- 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] Reviewing freeze map code
On 2016-06-06 16:41:19 -0400, Robert Haas wrote:
> I really don't understand how you can not weigh in on the original
> thread leading up to my mid-March commits and say "hey, this needs a
> better testing tool", and then when you finally get around to
> reviewing it in May, I'm supposed to drop everything and write one
> immediately.
Meh. Asking you to "drop everything" and starting to push a month later
are very different things. The reason I'm pushing is because this atm
seems likely to slip enough that we'll decide "can't do this for
9.6". And I think that'd be seriously bad.
> Why do you get two months from the time of commit to weigh in but I
> get no time to respond?
Really? You've started to apply pressure to fix things days after
they've been discovered. It's been a month.
> For my part, I thought I *had*
> written a testing tool - that's what pg_visibility is and that's what
> I used to test the feature before committing it.
I think looking only at page level data, and not at row level data is is
insufficient. And I think we need to make $tool output the data in a way
that only returns data if things are wrong (that can be a pre-canned
query).
> Now, you think that's not good enough, and I respect your opinion, but
> it's not as if you said this back when this was being committed. Or
> at least if you did, I don't remember it.
I think I mentioned testing ages ago, but not around the commit, no. I
kind of had assumed that it was there. I don't think that's really
relevant though. Backend flushing was discussed and benchmarked over
months as well; and while I don't agree with your, conclusion it's
absolutely sane of you to push for changing the default on that; even if
you didn't immediately push back.
> I know there's a patch. Both Tom and I are skeptical about whether it
> adds value, and I really don't think you've spelled out in as much
> detail why you think it will help as I have why I think it won't.
The primary reason I think it'll help because it allows users/testers to
run a simple one-line command (VACUUM (scan_all);)in their database, and
they'll get a clear "WARNING: XXX is bad" message if something's broken,
and nothing if things are ok. Vacuum isn't a bad place for that,
because it'll be the place that removes dead item pointers and such if
things were wrongly labeled; and because we historically have emitted
warnings from there. The more complex stuff we ask testers to run, the
less likely it is that they'll actually do that.
I'd also be ok with adding & documenting (beta release notes)
CREATE EXTENSION pg_visibility;
SELECT relname FROM pg_class WHERE relkind IN ('r', 'm') AND NOT
pg_check_visibility(oid);
or something olong those lines.
Greetings,
Andres Freund
--
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] Reviewing freeze map code
On Mon, Jun 6, 2016 at 5:06 PM, Andres Freund wrote:
> On 2016-06-06 16:41:19 -0400, Robert Haas wrote:
>> I really don't understand how you can not weigh in on the original
>> thread leading up to my mid-March commits and say "hey, this needs a
>> better testing tool", and then when you finally get around to
>> reviewing it in May, I'm supposed to drop everything and write one
>> immediately.
>
> Meh. Asking you to "drop everything" and starting to push a month later
> are very different things. The reason I'm pushing is because this atm
> seems likely to slip enough that we'll decide "can't do this for
> 9.6". And I think that'd be seriously bad.
To be clear, I'm not objecting to you pushing on this. I just think
your tone sounds a bit, uh, antagonized.
>> Why do you get two months from the time of commit to weigh in but I
>> get no time to respond?
>
> Really? You've started to apply pressure to fix things days after
> they've been discovered. It's been a month.
Yes, it would have been nice if I had gotten to this one sooner. But
it's not like you said "hey, hurry up" before I started working on it.
You waited until I did start working on it and *then* complained that
I didn't get to it sooner. I cannot rewind time.
>> For my part, I thought I *had*
>> written a testing tool - that's what pg_visibility is and that's what
>> I used to test the feature before committing it.
>
> I think looking only at page level data, and not at row level data is is
> insufficient. And I think we need to make $tool output the data in a way
> that only returns data if things are wrong (that can be a pre-canned
> query).
OK. I didn't think that was necessary, but it sure can't hurt.
>> I know there's a patch. Both Tom and I are skeptical about whether it
>> adds value, and I really don't think you've spelled out in as much
>> detail why you think it will help as I have why I think it won't.
>
> The primary reason I think it'll help because it allows users/testers to
> run a simple one-line command (VACUUM (scan_all);)in their database, and
> they'll get a clear "WARNING: XXX is bad" message if something's broken,
> and nothing if things are ok. Vacuum isn't a bad place for that,
> because it'll be the place that removes dead item pointers and such if
> things were wrongly labeled; and because we historically have emitted
> warnings from there. The more complex stuff we ask testers to run, the
> less likely it is that they'll actually do that.
OK, now I understand. Let's see if there is general agreement on this
and then we can decide how to proceed. I think the main danger here
is that people will think that this option is more useful than it
really is and start using it in all kinds of cases where it isn't
really necessary in the hopes that it will fix problems it really
can't fix. I think we need to write the documentation in such a way
as to be deeply discouraging to people who might otherwise be prone to
unwarranted optimism. Otherwise, 5 years from now, we're going to be
fielding complaints from people who are unhappy that there's no way to
make autovacuum run with (even_frozen_pages true).
> I'd also be ok with adding & documenting (beta release notes)
> CREATE EXTENSION pg_visibility;
> SELECT relname FROM pg_class WHERE relkind IN ('r', 'm') AND NOT
> pg_check_visibility(oid);
> or something olong those lines.
That wouldn't be too useful as-written in my book, because it gives
you no detail on what exactly the problem was. Maybe it could be
"pg_check_visibility(regclass) RETURNS SETOF tid", where the returned
TIDs are non-frozen TIDs on frozen pages. Then I think something like
this would work:
SELECT c.oid, pg_check_visibility(c.oid) FROM pg_class WHERE relkind
IN ('r', 't', 'm');
If you get any rows back, you've got trouble.
--
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] Reviewing freeze map code
Hi,
On 2016-06-06 17:00:19 -0400, Robert Haas wrote:
> 1. I think it is pretty misleading to say that those checks aren't
> reachable any more. It's not like we freeze every page when we mark
> it all-visible.
True. What I mean is that you can't force the checks (and some that I
think should be added) to occur anymore. Once a page is frozen it'll be
kinda hard to predict whether vacuum touches it (due to the skip logic).
> 2. With the new pg_visibility extension, you can actually check the
> same thing that first warning checks like this:
>
> select * from pg_visibility('t1'::regclass) where all_visible and not
> pd_all_visible;
Right, but not the second.
> IMHO, that's a substantial improvement over running VACUUM and
> checking whether it spits out a WARNING.
I think it's a mixed bag. I do think that WARNINGS are a lot easier to
understand for a casual user/tester; rather than having to write/copy
queries which return results where you don't know what the expected
result is. I agree that it's better to have that in a non-modifying way
- although I'm afraid atm it's not really possible to do a
HeapTupleSatisfies* without modifications :(.
> 3. If you think there are analogous checks that I should add for the
> frozen case, or that you want to add yourself, please say what they
> are specifically. I *did* think about it when I wrote that code and I
> didn't see how to make it work. If I had, I would have added them.
> The whole point of review here is, hopefully, to illuminate what
> should have been done differently - if I'd known how to do it better,
> I would have done so. Provide an idea, or better yet, provide a
> patch. If you see how to do it, coding it up shouldn't be the hard
> part.
I think it's pretty important (and not hard) to add a check for
(all_frozen_according_to_vm && has_unfrozen_tuples). Checking for
VM_ALL_FROZEN && !VM_ALL_VISIBLE looks worthwhile as well, especially as
we could check that always, without a measurable overhead. But the
former primarily makes sense if we have a way to force the check to
occur in a way that's not dependant on the state of neighbouring pages.
Greetings,
Andres Freund
--
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] Reviewing freeze map code
Hi,
On 2016-06-06 17:22:38 -0400, Robert Haas wrote:
> > I'd also be ok with adding & documenting (beta release notes)
> > CREATE EXTENSION pg_visibility;
> > SELECT relname FROM pg_class WHERE relkind IN ('r', 'm') AND NOT
> > pg_check_visibility(oid);
> > or something olong those lines.
>
> That wouldn't be too useful as-written in my book, because it gives
> you no detail on what exactly the problem was.
True. I don't think that's a big issue though, because we'd likely want
a lot more detail after a report anyway; to analyze things properly.
> Maybe it could be
> "pg_check_visibility(regclass) RETURNS SETOF tid", where the returned
> TIDs are non-frozen TIDs on frozen pages. Then I think something like
> this would work:
>
> SELECT c.oid, pg_check_visibility(c.oid) FROM pg_class WHERE relkind
> IN ('r', 't', 'm');
>
> If you get any rows back, you've got trouble.
That'd work too; with the slight danger of returning way too much data.
- Andres
--
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] Problem with dumping bloom extension
On Tue, Jun 7, 2016 at 12:01 AM, Robert Haas wrote: > On Fri, Jun 3, 2016 at 12:31 PM, Stephen Frost wrote: >>> Stephen, something is smelling wrong in checkExtensionMembership() >>> since 5d58999, an access method does not have ACLs and I would have >>> expected that when this routine is invoked in >>> selectDumpableAccessMethod() the object is not selected as dumpable. >> >> Yeah, I saw this also and was going to look into it. >> >> I suspect the issue may actually be that dumpAccessMethod() wasn't ever >> updated to have the appropriate conditionals for each of the components >> of the object. >> >> Specifically, there should be if statements along the lines of: >> >> if (aminfo->dobj.dump & DUMP_COMPONENT_DEFINITION) >> ArchiveEntry ... >> >> if (aminfo->dobj.dump & DUMP_COMPONENT_COMMENT) >> dumpComment() >> >> towards the end of dumpAccessMethod(). >> >> I'm not 100% sure that addresses this, but it definitely needs to be >> fixed also. I'll take care of it in the next few days. >> >> I'll also look more directly into what's going on here this weekend when >> I've got a bit more time to do so. > > It seems important to get this fixed. I added it to the open items list. I added already it as " Access methods created with extensions are dumped individually ". That's not specific to bloom. -- 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] COMMENT ON, psql and access methods
Michael Paquier wrote: > On Thu, Jun 2, 2016 at 3:42 PM, Michael Paquier > wrote: > > On Thu, Jun 2, 2016 at 1:00 PM, Michael Paquier > > wrote: > >> I have added an open item for 9.6 regarding this patch, that would be > >> good to complete this work in this release for consistency with the > >> other objects. > > > > Doh. I forgot to update psql --help. > > And Query_for_list_of_access_methods was defined more or less twice, > the one of my patch having an error... In looking at the DROP ACCESS METHOD completion I noticed that the words_after_create gadget is a bit buggy: for things with more than one word in the thing name (such as MATERIALIZED VIEW, USER MAPPING FOR, EVENT TRIGGER among others) the "query/squery"-based completion isn't triggered, because the loop at the end of psql_completion only considers a single word (using strcmp against prev_wd), which obviously doesn't match the multiple-word specifier in the struct. Some things such as EVENT TRIGGER and MATERIALIZED VIEW have specialized code that does the actual work; the latter specifies a query in words_after_create, but it's dead code. As a probably separate but related bug, CREATE USER MAPPING FOR stops working after you tab-complete the USER in it. Lastly, there is an entry for CONFIGURATION which also doesn't work: if you enter "DROP " it doesn't complete CONFIGURATION, but if you enter "DROP CONFIGURATION " then it shows a list of text search configurations, which is not a valid command. To conclude, so far as I can tell, your patch (for DROP AM completion) is fine, but the existing code has some minor flags which we could just as well ignore for now, but could be improved in the future. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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] hstore: add hstore_length function
Thanks for the review, Fabrízio. Attached is the updated patch, rebased and tested against master. I integrated feedback from your first point and am no longer HS_POLLUTE'ing the namespace for the new function. With regards to your second point- I've been maintaining consistency with the rest of the hstore module. Hstore's _size is internally stored as a uint, but all uses of HS_COUNT across the feature end up stored in a signed int. I could only find (grep) a few occurrences of PG_RETURN_UINT32 across the entire codebase, and none in the hstore module. If there's strong consensus for change, though, I'm happy to do so. Thanks, Korbin Hoffman On Mon, Jun 6, 2016 at 1:23 PM, Fabrízio de Royes Mello wrote: > > > On Fri, Jun 3, 2016 at 7:58 AM, Korbin Hoffman wrote: >> >> Hi there- >> >> I've attached a small patch exposing HS_COUNT to the user as >> "hstore_length(hstore)". Documentation, upgrade sql files, and a few >> tests are also included. >> >> Almost every hstore function calls HS_COUNT, but I couldn't determine >> if there was a reason this exposure didn't already exist. >> >> Without this function, I've been converting an hstore into an array >> and then counting it - a more expensive operation (~30-40% slower than >> SELECTing the hstore itself in a few of my tests). >> >> I will add this thread and patch to the next Commitfest. >> > > Something goes wrong when applying against master: > > $ git apply ~/Downloads/hstore_length-v1.patch > error: contrib/hstore/Makefile: already exists in working directory > error: contrib/hstore/expected/hstore.out: already exists in working > directory > error: contrib/hstore/hstore--1.3.sql: already exists in working directory > error: contrib/hstore/hstore.control: already exists in working directory > error: contrib/hstore/hstore_op.c: already exists in working directory > error: contrib/hstore/sql/hstore.sql: already exists in working directory > error: doc/src/sgml/hstore.sgml: already exists in working directory > > > Anyway I have some comments: > > 1) I don't see any reason to add this sort of thing if you're adding a new > function > > + HSTORE_POLLUTE(hstore_length, length); > > > 2) Shouldn't this declaration use 'uint32' instead of 'int' ?? > > + int count = HS_COUNT(hs); > + > + PG_RETURN_INT32(count); > > maybe > > + uint32 count = HS_COUNT(hs); > + > + PG_RETURN_UINT32(count); > > 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 hstore_length-v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] parallel workers and client encoding
There appears to be a problem with how client encoding is handled in the communication from parallel workers. In a parallel worker, the client encoding setting is inherited from its creating process as part of the GUC setup. So any plain-text stuff the parallel worker sends to its leader is actually converted to the client encoding. Since most data is sent in binary format, the plain-text provision applies mainly to notice and error messages. At the other end, error messages are parsed using pq_parse_errornotice(), which internally uses routines that were meant for communication from the client, and therefore will convert everything back from the client encoding to the server encoding. So this whole thing actually happens to work as long as round tripping is possible between the involved encodings. In cases where it isn't, it's still hard to notice the difference because depending on whether you get a parallel plan or not, the following happens: not parallel: conversion error happens between server and client, client sees an error message about that parallel: conversion error happens between worker and leader, worker generates an error message about that, sends it to leader, leader forwards it to client The client sees the same error message in both cases. To construct a case where this makes a difference, the leader has to be set up to catch certain errors. Here is an example: """ create table test1 (a int, b text); truncate test1; insert into test1 values (1, 'a'); create or replace function test1() returns text language plpgsql as $$ declare res text; begin perform from test1 where a = test2(); return res; exception when division_by_zero then return 'boom'; end; $$; create or replace function test2() returns int language plpgsql parallel safe as $$ begin raise division_by_zero using message = 'Motörhead'; return 1; end $$; set force_parallel_mode to on; select test1(); """ With client_encoding = server_encoding, this will return a single row 'boom'. But with, say, database encoding UTF8 and PGCLIENTENCODING=KOI8R, it will error: ERROR: 22P05: character with byte sequence 0xef 0xbe 0x83 in encoding "UTF8" has no equivalent in encoding "KOI8R" CONTEXT: parallel worker (Note that changing force_parallel_mode does not force replanning in plpgsql, so if you run test1() first before setting force_parallel_mode, then you won't get the error.) Attached is a patch to illustrates how this could be fixed. There might be similar issues elsewhere. The notification propagation in particular could be affected. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c index 934dba8..d2105ec 100644 --- a/src/backend/access/transam/parallel.c +++ b/src/backend/access/transam/parallel.c @@ -775,6 +775,7 @@ HandleParallelMessage(ParallelContext *pcxt, int i, StringInfo msg) ErrorData edata; ErrorContextCallback errctx; ErrorContextCallback *save_error_context_stack; + int save_client_encoding; /* * Rethrow the error using the error context callbacks that @@ -787,9 +788,14 @@ HandleParallelMessage(ParallelContext *pcxt, int i, StringInfo msg) errctx.previous = pcxt->error_context_stack; error_context_stack = &errctx; + save_client_encoding = pg_get_client_encoding(); + SetClientEncoding(GetDatabaseEncoding()); + /* Parse ErrorResponse or NoticeResponse. */ pq_parse_errornotice(msg, &edata); + SetClientEncoding(save_client_encoding); + /* Death of a worker isn't enough justification for suicide. */ edata.elevel = Min(edata.elevel, ERROR); @@ -989,6 +995,7 @@ ParallelWorkerMain(Datum main_arg) StartTransactionCommand(); RestoreGUCState(gucspace); CommitTransactionCommand(); + SetClientEncoding(GetDatabaseEncoding()); /* Crank up a transaction state appropriate to a parallel worker. */ tstatespace = shm_toc_lookup(toc, PARALLEL_KEY_TRANSACTION_STATE); -- 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] Parallel pg_dump's error reporting doesn't work worth squat
At Mon, 06 Jun 2016 11:12:14 -0400, Tom Lane wrote in <17504.1465225...@sss.pgh.pa.us> > Kyotaro HORIGUCHI writes: > > At Fri, 03 Jun 2016 09:44:30 -0400, Tom Lane wrote in > > <11515.1464961...@sss.pgh.pa.us> > >> I think that this one: > >>> If the target thread is allocating memory from the heap, the heap > >>> lock will not be released. > >> is potentially a hazard, which is why I made sure to use write_stderr > >> later on in the console interrupt handler. Your original suggestion > >> to use write_msg would end up going through fprintf, which might well > >> use malloc internally. (It's possible that Windows' version of write() > >> could too, I suppose, but that's probably as low-level as we are > >> going to get.) > > > I have to admit that I forgot about the possible malloc's, and > > PQcancel() can be blocked from the same reason. > > Uh, what? PQcancel is very carefully coded so that it's safe to use > in a signal handler. If it's doing mallocs someplace, that would be > surprising. PQcancel is disigned to run in a signal handler on *Linux*, but the discussion here is that the equivalent of send/recv and the similars on Windows can be blocked by the TerminateThread'ed thread via heap lock. > > If the issue to be settled here is the unwanted error messages, > > we could set a flag to instruct write_msg to sit silent. Anyway > > the workers should have been dead that time so discarding any > > error messages don't matter. > > What do you think about this? > > This is really ugly and I'm unconvinced that it fixes anything. > write_msg is hardly the only place in a worker thread that might > be doing malloc's; moreover, preventing workers from entering it > after we start a shutdown does nothing for workers that might be > in it already. Yeah, it's ugly. But if we assume write() can be blocked, the similar system calls used within PQcancel can be blocked, too. If we don't assume so, using write instead of write_msg would do. The problem I think is we don't have (or they don't offer?) enough knowlegde about the inside of Windows APIs. 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
Re: [HACKERS] installcheck failing on psql_crosstab
On Tue, Jun 7, 2016 at 12:28 AM, Alvaro Herrera wrote: > Tom Lane wrote: >> Alvaro Herrera writes: > >> > I can't imagine that the server is avoiding hash aggregation on a 1MB >> > work_mem limit for data that's a few dozen of bytes. Is it really doing >> > that? >> >> Yup: > > Aha. Thanks for testing. > >> Now that you mention it, this does seem a bit odd, although I remember >> that there's a pretty substantial fudge factor in there when we have >> no statistics (which we don't in this example). If I ANALYZE ctv_data >> then it sticks to the hashagg plan all the way down to 64kB work_mem. > > Hmm, so we could solve the complaint by adding an ANALYZE. I'm open to > that; other opinions? We could just enforce work_mem to 64kB and then reset it. -- 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] OUT parameter and RETURN table/setof
Existing application code written to call function in Oracle which return no.of rows in out parameter and return-values is cursor-result this need migrate to PostgreSQL, need help here example: (actual function declaration only) *Oracle:* CREATE OR REPLACE PROCEDURE sc_getapppermissionlist ( v_role_ids IN VARCHAR2, v_rowsfound OUT INTEGER, result_cursor1 OUT SYS_REFCURSOR ) ... *PostgreSQL:* *method 1*: CREATE OR REPLACE PROCEDURE sc_getapppermissionlist ( v_role_ids IN VARCHAR, v_rowsfound OUT INTEGER, result_cursor1 OUT REFCURSOR ) ... but this approach issue is, need to do in BEGIN - END block inside with FETCH ALL IN "" - here we need/think common approach for database *method 2:* CREATE OR REPLACE PROCEDURE sc_getapppermissionlist ( v_role_ids IN VARCHAR, v_rowsfound OUT INTEGER) RETURNS TABLE/SETOF ... this approach is not working Thanks Sridhar OpenText On Mon, Jun 6, 2016 at 5:57 PM, David G. Johnston < david.g.johns...@gmail.com> wrote: > On Mon, Jun 6, 2016 at 7:17 AM, Sridhar N Bamandlapally < > sridhar@gmail.com> wrote: > >> Hi >> >> Is there any option in PGPLSQL which can RETURNS table or SETOF rows >> along with an OUT parameter? >> >> > No, there would be no point given the internals of how functions work. > > What is it you are trying to do? > > David J. > > >
Re: [HACKERS] installcheck failing on psql_crosstab
On Tue, Jun 7, 2016 at 12:31 PM, Michael Paquier
wrote:
> On Tue, Jun 7, 2016 at 12:28 AM, Alvaro Herrera
> wrote:
>> Tom Lane wrote:
>>> Alvaro Herrera writes:
>>
>>> > I can't imagine that the server is avoiding hash aggregation on a 1MB
>>> > work_mem limit for data that's a few dozen of bytes. Is it really doing
>>> > that?
>>>
>>> Yup:
>>
>> Aha. Thanks for testing.
>>
>>> Now that you mention it, this does seem a bit odd, although I remember
>>> that there's a pretty substantial fudge factor in there when we have
>>> no statistics (which we don't in this example). If I ANALYZE ctv_data
>>> then it sticks to the hashagg plan all the way down to 64kB work_mem.
>>
>> Hmm, so we could solve the complaint by adding an ANALYZE. I'm open to
>> that; other opinions?
>
> We could just enforce work_mem to 64kB and then reset it.
Or just set up work_mem to a wanted value for the duration of the run
of psql_crosstab. Attached is my proposal.
--
Michael
diff --git a/src/test/regress/expected/psql_crosstab.out b/src/test/regress/expected/psql_crosstab.out
index a9c20a1..57b68bb 100644
--- a/src/test/regress/expected/psql_crosstab.out
+++ b/src/test/regress/expected/psql_crosstab.out
@@ -10,6 +10,8 @@ VALUES
('v0','h4','dbl', -3, '2014-12-15'),
('v0',NULL,'qux', 5, '2014-07-15'),
('v1','h2','quux',7, '2015-04-04');
+-- ensure plan consistency across the test
+SET work_mem = '64kB';
-- running \crosstabview after query uses query in buffer
SELECT v, EXTRACT(year FROM d), count(*)
FROM ctv_data
@@ -127,8 +129,8 @@ GROUP BY v, h ORDER BY h,v
\crosstabview v h i
v | h0 | h1 | h2 | h4 | #null#
+++++
- v1 | #null# || 3 +||
-||| 7 ||
+ v1 | #null# || 7 +||
+||| 3 ||
v2 || 3 |||
v0 |||| 4 +| 5
|||| -3 |
@@ -143,8 +145,8 @@ FROM ctv_data GROUP BY v, h ORDER BY h,v
+--+-+-
h0 | baz | |
h1 | | bar |
- h2 | foo +| |
-| quux | |
+ h2 | quux+| |
+| foo | |
h4 | | | qux+
| | | dbl
| | | qux
@@ -156,8 +158,8 @@ FROM ctv_data GROUP BY v, h ORDER BY h,v
\crosstabview 1 "h" 4
v | h0 | h1 | h2 | h4 |
+-+-+--+-+-
- v1 | baz | | foo +| |
-| | | quux | |
+ v1 | baz | | quux+| |
+| | | foo | |
v2 | | bar | | |
v0 | | | | qux+| qux
| | | | dbl |
diff --git a/src/test/regress/sql/psql_crosstab.sql b/src/test/regress/sql/psql_crosstab.sql
index 43c959b..cb43556 100644
--- a/src/test/regress/sql/psql_crosstab.sql
+++ b/src/test/regress/sql/psql_crosstab.sql
@@ -12,6 +12,9 @@ VALUES
('v0',NULL,'qux', 5, '2014-07-15'),
('v1','h2','quux',7, '2015-04-04');
+-- ensure plan consistency across the test
+SET work_mem = '64kB';
+
-- running \crosstabview after query uses query in buffer
SELECT v, EXTRACT(year FROM d), count(*)
FROM ctv_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] COMMENT ON, psql and access methods
On Tue, Jun 7, 2016 at 7:35 AM, Alvaro Herrera wrote: > In looking at the DROP ACCESS METHOD completion I noticed that the > words_after_create gadget is a bit buggy: for things with more than one > word in the thing name (such as MATERIALIZED VIEW, USER MAPPING FOR, > EVENT TRIGGER among others) the "query/squery"-based completion isn't > triggered, because the loop at the end of psql_completion only considers > a single word (using strcmp against prev_wd), which obviously doesn't > match the multiple-word specifier in the struct. Some things such as > EVENT TRIGGER and MATERIALIZED VIEW have specialized code that does the > actual work; the latter specifies a query in words_after_create, but > it's dead code. As a probably separate but related bug, CREATE USER > MAPPING FOR stops working after you tab-complete the USER in it. Yes, that's not new... > Lastly, there is an entry for CONFIGURATION which also doesn't work: > if you enter "DROP " it doesn't complete CONFIGURATION, but if you > enter "DROP CONFIGURATION " then it shows a list of text search > configurations, which is not a valid command. This is not a new issue as well. Even before the tab completion refactoring things are behaving this way. There is much room for improvements. The refactoring makes back-patching a bit more difficult, so we may just want to get those improvements in 9.6~ based on the lack of complaints regarding that. > To conclude, so far as I can tell, your patch (for DROP AM completion) > is fine, but the existing code has some minor flags which we could just > as well ignore for now, but could be improved in the future. Thanks! -- 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] IPv6 link-local addresses and init data type
On Sat, Jun 4, 2016 at 1:50 PM, Tom Lane wrote: > Robert Haas writes: >> Yeah, but what if somebody doesn't want to store scopes? > > [ shrug... ] We can invent a "strip_scope()" sort of function, > analogous to the weight-stripping function for tsvectors, or several > other examples. That's a really lame excuse for not being *able* > to store scopes. > >> ff::1%fourscoreandsevenyearsagoourforefathersbroughtforthonthiscontinentanewnationconceivedinlibertyanddedicatedtothepropositionthatlallmenarecreatedequal > > I have not looked at the spec, but I wouldn't be surprised if there > were an upper limit on the length of valid scope names. I am not able to find any link that suggests the maximum length of the scope id. >From [1], I came to know that, how the scope id is formed in different >operating systems. windows - fe80::3%1 unix systems - fe80::3%eth0 >From [2], the maximum interface name allowed in linux is 16 bytes and windows 256 bytes. Any way for windows the zone id is a number index to the interface. I added another character array of 256 member into inet_struct as a last member to store the zone id. Modified the inet_cidr_pton_ipv6 function to handle '%'. Currently all the printable characters are treated as zone id's. I will restrict this to only alphabets and numbers. Here I attached POC patch that adds the support of storing ipv6 with scope id in the inet datatype. Approach of adding the member to the structure and storing is fine or any better way to handle the same? If we come to an conclusion on the approach, i will add the zone support for everything and send the patch. How about the following case, Do we treat them as same or different? select 'fe80::%eth1'::inet = 'fe80::%ETH1'::inet; fe80::%2/64 is only treated as the valid address but not other way as fe80::/64%2. Do we need to throw an error in this case or just ignore. [1] - https://en.wikipedia.org/wiki/IPv6_address#Link-local_addresses_and_zone_indices [2] - http://stackoverflow.com/questions/24932172/what-length-can-a-network-interface-name-have Regards, Hari Babu Fujitsu Australia ipv6_scopeid_poc.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] parallel.c is not marked as test covered
Thanks for this patch. I have reviewed it: On Sun, May 15, 2016 at 12:53:13PM +, Clément Prévost wrote: > After some experiments, I found out that, for my setup (9b7bfc3a88ef7b), a > parallel seq scan is used given both parallel_setup_cost > and parallel_tuple_cost are set to 0 and given that the table is at least 3 > times as large as the biggest test table tenk1. That worked because it pushed the table over the create_plain_partial_paths() 1000-block (8 MiB) threshold; see http://www.postgresql.org/message-id/26842.1462941...@sss.pgh.pa.us While the way you tested this is not wrong, I recommend instead querying an inheritance parent if that achieves no less test coverage. "SELECT count(*) FROM a_star" gets a parallel plan under your cost parameters. That avoids building and discarding a relatively-large table. > The attached patch is a regression test using this method that is > reasonably small and fast to run. I also hid the workers count from the > explain output when costs are disabled as suggested by Tom Lane and Robert > Haas on this same thread ( > http://www.postgresql.org/message-id/CA+TgmobBQS4ss3+CwoZOKgbsBqKfRndwc=hlialaep5axqc...@mail.gmail.com > ). It seems wrong to me for COSTS OFF to disable output that is not a cost estimate. file_fdw did that, but I don't want to proliferate that loose interpretation of COSTS OFF. Unlike the example in the linked-to message, this test won't experience variable worker count. For the query you used and for the a_star query, low input size makes the planner request exactly one worker. If later planner enhancements give this query a configuration-specific worker count, the test could capture EXPLAIN output with PL/pgSQL and compare it to a regex. > Testing under these conditions does not test the planner job at all but at > least some parallel code can be run on the build farm and the test suite > gets 2643 more lines and 188 more function covered. Nice. > --- a/src/test/regress/parallel_schedule > +++ b/src/test/regress/parallel_schedule > @@ -79,7 +79,7 @@ ignore: random > # -- > # Another group of parallel tests > # -- > -test: select_into select_distinct select_distinct_on select_implicit > select_having subselect union case join aggregates transactions random > portals arrays btree_index hash_index update namespace prepared_xacts delete > +test: select_into select_distinct select_distinct_on select_implicit > select_having subselect union case join aggregates transactions random > portals arrays btree_index hash_index update namespace prepared_xacts delete > select_parallel Add this to serial_schedule, too. > --- /dev/null > +++ b/src/test/regress/sql/select_parallel.sql > @@ -0,0 +1,22 @@ > +-- > +-- PARALLEL > +-- > + > +-- setup parallel test > +create unlogged table tenk_parallel as table tenk1 with no data; > +set parallel_setup_cost=0; > +set parallel_tuple_cost=0; > + > +-- create a table with enough data to trigger parallel behavior > +insert into tenk_parallel > + select tenk1.* from tenk1, generate_series(0,2); > + > +explain (costs off) > + select count(*) from tenk_parallel; > +select count(*) from tenk_parallel; As of today, "make installcheck" passes with "default_transaction_isolation = serializable" in postgresql.conf. Let's preserve that property. You could wrap the parallel queries in "begin isolation level repeatable read;" ... "commit;", or you could SET default_transaction_isolation itself. > + > +-- cleanup > +drop table tenk_parallel; > +set parallel_setup_cost to default; > +set parallel_tuple_cost to default; > + Remove the trailing blank line; it triggers a "git diff --check" diagnostic. [Official open item status update: I expect to be able to review the next patch version within a day or two of it becoming available. I will update this thread by 2016-06-14 09:00 UTC if not earlier.] -- 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] [BUGS] BUG #14155: bloom index error with unlogged table
On Fri, Jun 3, 2016 at 11:25 PM, Tom Lane wrote:
> Michael Paquier writes:
>> Actually, the docs could be more polished.
>
> I think the docs could stand to be rewritten from scratch ;-). But
> upthread there was an offer to work on them if we made the code behavior
> saner. I've done the latter part, I don't want to do the former.
I have finally given a shot at improving the docs with the attached.
Comments are welcome.
--
Michael
diff --git a/doc/src/sgml/bloom.sgml b/doc/src/sgml/bloom.sgml
index 8667763..c1e204f 100644
--- a/doc/src/sgml/bloom.sgml
+++ b/doc/src/sgml/bloom.sgml
@@ -8,43 +8,38 @@
- bloom is a module that implements an index access method. It comes
- as an example of custom access methods and generic WAL record usage. But it
- is also useful in itself.
+ bloom provides an index access method usable as a
+ http://en.wikipedia.org/wiki/Bloom_filter";>Bloom filter.
-
- Introduction
-
-
- The implementation of a
- http://en.wikipedia.org/wiki/Bloom_filter";>Bloom filter
- allows fast exclusion of non-candidate tuples via signatures.
- Since a signature is a lossy representation of all indexed attributes,
- search results must be rechecked using heap information.
- The user can specify signature length in bits (default 80, maximum 4096)
- and the number of bits generated for each index column (default 2,
- maximum 4095).
-
+
+ A bloom filter is a space-efficient data structure that is used to test
+ whether an element is a member of a set. In the case of an index access
+ method, it allows fast exclusion of non-candidate tuples via signatures
+ whose size is calculated in bits and is controllable at index creation.
+
-
- This index is useful if a table has many attributes and queries include
- arbitrary combinations of them. A traditional btree index is
- faster than a bloom index, but it can require many indexes to support all
- possible queries where one needs only a single bloom index. A Bloom index
- supports only equality comparison. Since it's a signature file, and not a
- tree, it always must be read fully, but sequentially, so that index search
- performance is constant and doesn't depend on a query.
-
-
+
+ A signature is a lossy representation of all indexed attributes and
+ so it can report false positives (it is reported that an element exists in
+ the set, however it does not), so search results must be rechecked using
+ heap information.
+
+
+
+ This type of index is useful if a table has many attributes and queries
+ include arbitrary combinations of them. A traditional btree
+ index is faster than a bloom index, but it can require many indexes to
+ support all possible queries where one needs only a single bloom index
+ (default 80, maximum 4096).
+
Parameters
- bloom indexes accept the following parameters in the
- WITH
- clause.
+ A bloom index accepts the following parameters in its
+ WITH clause.
@@ -52,7 +47,8 @@
length
- Length of signature in bits
+ Length of signature in bits. Default is 80 and maximum is
+ 4096.
@@ -62,7 +58,9 @@
col1 — col32
- Number of bits generated for each index column
+ Number of bits generated for each index column. Each parameter refers
+ to the number of the column for the relation on which an index is
+ defined.
@@ -82,91 +80,88 @@ CREATE INDEX bloomidx ON tbloom USING bloom (i1,i2,i3)
- Here, we created a bloom index with a signature length of 80 bits,
- and attributes i1 and i2 mapped to 2 bits, and attribute i3 mapped to 4 bits.
+ Here, a bloom index is created with a signature length of 80 bits, and
+ attributes i1 and i2 mapped to 2 bits, and attribute i3 mapped to 4 bits.
- Here is a fuller example of index definition and usage:
+ Here is a more complete example of index definition and usage, a bloom
+ index representing first the advantage to be more space-efficient
+ than an equivalent btree index:
-CREATE TABLE tbloom AS
-SELECT
-random()::int as i1,
-random()::int as i2,
-random()::int as i3,
-random()::int as i4,
-random()::int as i5,
-random()::int as i6,
-random()::int as i7,
-random()::int as i8,
-random()::int as i9,
-random()::int as i10,
-random()::int as i11,
-random()::int as i12,
-random()::int as i13
-FROM
-generate_series(1,1000);
-CREATE INDEX bloomidx ON tbloom USING
- bloom (i1, i2, i3, i4, i5, i6, i7, i8, i9, i10, i11, i12);
-SELECT pg_relation_size('bloomidx');
-CREATE index btree_idx ON tbloom(i1,i2,i3,i4,i5,i6,i7,i8,i9,i10,i11,i12);
-SELECT pg_relation_size('btree_idx');
-
-
-
-=# EXPLAIN ANALYZE SELECT * FROM tbloom WHERE i2 = 20 AND i10 = 15;
- QUERY PLAN
---
Re: [HACKERS] Typos/Questions in bloom documentation
On Fri, Apr 22, 2016 at 1:25 AM, David G. Johnston wrote: > On Wed, Apr 20, 2016 at 9:18 PM, Amit Langote > wrote: >> I agree it's unclear. Does the following make it any better (updated >> patch attached): I have sent a patch to rework the docs here: http://www.postgresql.org/message-id/cab7npqqb8dcfmy1uodmijoszdhbfox-us-uw6rfyrzhpeib...@mail.gmail.com This may interest people here. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Problem with dumping bloom extension
On Tue, Jun 7, 2016 at 6:55 AM, Michael Paquier wrote: > On Tue, Jun 7, 2016 at 12:01 AM, Robert Haas wrote: >> On Fri, Jun 3, 2016 at 12:31 PM, Stephen Frost wrote: Stephen, something is smelling wrong in checkExtensionMembership() since 5d58999, an access method does not have ACLs and I would have expected that when this routine is invoked in selectDumpableAccessMethod() the object is not selected as dumpable. >>> >>> Yeah, I saw this also and was going to look into it. >>> >>> I suspect the issue may actually be that dumpAccessMethod() wasn't ever >>> updated to have the appropriate conditionals for each of the components >>> of the object. >>> >>> Specifically, there should be if statements along the lines of: >>> >>> if (aminfo->dobj.dump & DUMP_COMPONENT_DEFINITION) >>> ArchiveEntry ... >>> >>> if (aminfo->dobj.dump & DUMP_COMPONENT_COMMENT) >>> dumpComment() >>> >>> towards the end of dumpAccessMethod(). >>> >>> I'm not 100% sure that addresses this, but it definitely needs to be >>> fixed also. I'll take care of it in the next few days. >>> >>> I'll also look more directly into what's going on here this weekend when >>> I've got a bit more time to do so. >> >> It seems important to get this fixed. I added it to the open items list. Stephen, are you working on a patch or should I get one out of my pocket? That's something we should get fixed quickly. We need as well to be careful with the support for COMMENT with access methods, the current consensus on the matter is that it will be soon committed. -- 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] Problem with dumping bloom extension
On Fri, Jun 03, 2016 at 12:31:44PM -0400, Stephen Frost wrote: > * Michael Paquier (michael.paqu...@gmail.com) wrote: > > On Fri, Jun 3, 2016 at 8:57 PM, Thom Brown wrote: > > > If a database with the bloom extension installed is dumped and restored, > > > there's an error with the access method creation: > > > > > > createdb bloomtest > > > psql -c 'CREATE EXTENSION bloom;' bloomtest > > > pg_dump -d bloomtest > ~/tmp/bloom.sql > > > createdb bloomtest2 > > > psql -d bloomtest2 -f ~/tmp/bloom.sql > > > > > > The output of the last command produces: > > > > > > "psql:/home/thom/tmp/bloom.sql:48: ERROR: access method "bloom" already > > > exists" > > > > > > So pg_dump shouldn't be dumping this access method as it's part of the > > > extension. > > > > Stephen, something is smelling wrong in checkExtensionMembership() > > since 5d58999, an access method does not have ACLs and I would have > > expected that when this routine is invoked in > > selectDumpableAccessMethod() the object is not selected as dumpable. > > Yeah, I saw this also and was going to look into it. [Action required within 72 hours. This is a generic notification.] The above-described topic is currently a PostgreSQL 9.6 open item. Stephen, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a 9.6 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within 72 hours of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping 9.6rc1. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
