Re: [HACKERS] Block level parallel vacuum

[Dilip Kumar]

On Fri, Oct 4, 2019 at 3:35 PM Dilip Kumar  wrote:
>
> On Fri, Oct 4, 2019 at 11:01 AM Amit Kapila  wrote:
> >
> > On Fri, Oct 4, 2019 at 10:28 AM Masahiko Sawada  
> > wrote:
> >>
> Some more comments..
> 1.
> + for (idx = 0; idx < nindexes; idx++)
> + {
> + if (!for_cleanup)
> + lazy_vacuum_index(Irel[idx], &stats[idx], vacrelstats->dead_tuples,
> +   vacrelstats->old_live_tuples);
> + else
> + {
> + /* Cleanup one index and update index statistics */
> + lazy_cleanup_index(Irel[idx], &stats[idx], vacrelstats->new_rel_tuples,
> +vacrelstats->tupcount_pages < vacrelstats->rel_pages);
> +
> + lazy_update_index_statistics(Irel[idx], stats[idx]);
> +
> + if (stats[idx])
> + pfree(stats[idx]);
> + }
>
> I think instead of checking for_cleanup variable for every index of
> the loop we better move loop inside, like shown below?
>
> if (!for_cleanup)
> for (idx = 0; idx < nindexes; idx++)
> lazy_vacuum_index(Irel[idx], &stats[idx], vacrelstats->dead_tuples,
> else
> for (idx = 0; idx < nindexes; idx++)
> {
> lazy_cleanup_index
> lazy_update_index_statistics
> ...
> }
>
> 2.
> +static void
> +lazy_vacuum_or_cleanup_indexes(LVRelStats *vacrelstats, Relation *Irel,
> +int nindexes, IndexBulkDeleteResult **stats,
> +LVParallelState *lps, bool for_cleanup)
> +{
> + int idx;
> +
> + Assert(!IsParallelWorker());
> +
> + /* no job if the table has no index */
> + if (nindexes <= 0)
> + return;
>
> Wouldn't it be good idea to call this function only if nindexes > 0?
>
> 3.
> +/*
> + * Vacuum or cleanup indexes with parallel workers. This function must be 
> used
> + * by the parallel vacuum leader process.
> + */
> +static void
> +lazy_parallel_vacuum_or_cleanup_indexes(LVRelStats *vacrelstats,
> Relation *Irel,
> + int nindexes, IndexBulkDeleteResult **stats,
> + LVParallelState *lps, bool for_cleanup)
>
> If you see this function there is no much common code between
> for_cleanup and without for_cleanup except these 3-4 statement.
> LaunchParallelWorkers(lps->pcxt);
> /* Create the log message to report */
> initStringInfo(&buf);
> ...
> /* Wait for all vacuum workers to finish */
> WaitForParallelWorkersToFinish(lps->pcxt);
>
> Other than that you have got a lot of checks like this
> + if (!for_cleanup)
> + {
> + }
> + else
> + {
> }
>
> I think code would be much redable if we have 2 functions one for
> vaccum (lazy_parallel_vacuum_indexes) and another for
> cleanup(lazy_parallel_cleanup_indexes).
>
> 4.
>  * of index scans performed.  So we don't use maintenance_work_mem memory for
>   * the TID array, just enough to hold as many heap tuples as fit on one page.
>   *
> + * Lazy vacuum supports parallel execution with parallel worker processes. In
> + * parallel lazy vacuum, we perform both index vacuuming and index cleanup 
> with
> + * parallel worker processes. Individual indexes are processed by one vacuum
>
> Spacing after the "." is not uniform, previous comment is using 2
> space and newly
> added is using 1 space.

Few more comments


1.
+static int
+compute_parallel_workers(Relation onerel, int nrequested, int nindexes)
+{
+ int parallel_workers;
+ bool leaderparticipates = true;

Seems like this function is not using onerel parameter so we can remove this.


2.
+
+ /* Estimate size for dead tuples -- PARALLEL_VACUUM_KEY_DEAD_TUPLES */
+ maxtuples = compute_max_dead_tuples(nblocks, true);
+ est_deadtuples = MAXALIGN(add_size(SizeOfLVDeadTuples,
+mul_size(sizeof(ItemPointerData), maxtuples)));
+ shm_toc_estimate_chunk(&pcxt->estimator, est_deadtuples);
+ shm_toc_estimate_keys(&pcxt->estimator, 1);
+
+ /* Finally, estimate VACUUM_KEY_QUERY_TEXT space */
+ querylen = strlen(debug_query_string);

for consistency with other comments change
VACUUM_KEY_QUERY_TEXT  to PARALLEL_VACUUM_KEY_QUERY_TEXT


3.
@@ -2888,6 +2888,8 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
  (!wraparound ? VACOPT_SKIP_LOCKED : 0);
  tab->at_params.index_cleanup = VACOPT_TERNARY_DEFAULT;
  tab->at_params.truncate = VACOPT_TERNARY_DEFAULT;
+ /* parallel lazy vacuum is not supported for autovacuum */
+ tab->at_params.nworkers = -1;

What is the reason for the same?  Can we explain in the comments?


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: The flinfo->fn_extra question, from me this time.

[Dent John]

On 22 Sep 2019, at 16:01, Tom Lane  wrote:Hi Tom,I don't know of anybody working on it.Okay. I had a look at this. I tried to apply Andre’s patch [1] from some time ago, but that turned out not so easy. I guess the code has moved on since. So I’ve attempted to re-invent the same spirit, stealing from his patch, and from how the tSRF code does things. The patch isn’t final, but it demonstrates a concept.However, given your comments below, I wonder if you might comment on the approach before I go further?(Patch is presently still against 12beta2.)You'd still need to be prepared to build a tuplestore,in case of rescan or backwards fetch; but […]I do recognise this. The patch teaches ExecMaterializesOutput() and ExecSupportsBackwardScan() that T_FunctionScan nodes don't materialise their output.(Actually, Andre’s patch did the educating of ExecMaterializesOutput() and ExecSupportsBackwardScan() — it’s not my invention.)I haven’t worked out how to easily demonstrate the backward scan case, but joins (which presumably are the typical cause of rescan) now yield an intermediate Materialize node.postgres=# explain (analyze, buffers) select * from unnest (array_fill ('scanner'::text, array[10])) t1, unnest (array_fill ('dummy'::text, array[1000])) t2 limit 100;                                                            QUERY PLAN                                                            -- Limit  (cost=0.01..1.36 rows=100 width=64) (actual time=0.009..0.067 rows=100 loops=1)   ->  Nested Loop  (cost=0.01..135.13 rows=1 width=64) (actual time=0.008..0.049 rows=100 loops=1)         ->  Function Scan on unnest t2  (cost=0.00..10.00 rows=1000 width=32) (actual time=0.003..0.006 rows=10 loops=1)         ->  Materialize  (cost=0.00..0.15 rows=10 width=32) (actual time=0.000..0.002 rows=10 loops=10)               ->  Function Scan on unnest t1  (cost=0.00..0.10 rows=10 width=32) (actual time=0.001..0.004 rows=10 loops=1) Planning Time: 127.875 ms Execution Time: 0.102 ms(7 rows)My point was that you can't simply remove the tuplestore-building codepath.  The exact boundary conditions for that might be negotiable.But I'd be very dubious of an assumption that re-running the functionwould be cheaper than building a tuplestore, regardless of whether it'ssafe.Understood, and I agree. I think it’s preferable to allow the planner control over when to explicitly materialise.But if I’m not wrong, at present, the planner doesn’t really trade-off the cost of rescan versus materialisation, but instead adopts a simple heuristic of materialising one or other side during a join. We can see this in the plans if the unnest()s are moved into the target list and buried in a subquery. For example:postgres=# explain (analyze, buffers) select * from (select unnest (array_fill ('scanner'::text, array[10]))) t1, (select unnest (array_fill ('dummy'::text, array[1000]))) t2 limit 100;                                                   QUERY PLAN                                                    - Limit  (cost=0.00..1.40 rows=100 width=64) (actual time=0.011..0.106 rows=100 loops=1)   ->  Nested Loop  (cost=0.00..140.21 rows=1 width=64) (actual time=0.010..0.081 rows=100 loops=1)         ->  ProjectSet  (cost=0.00..5.02 rows=1000 width=32) (actual time=0.004..0.024 rows=10 loops=1)               ->  Result  (cost=0.00..0.01 rows=1 width=0) (actual time=0.001..0.001 rows=1 loops=1)         ->  Materialize  (cost=0.00..0.22 rows=10 width=32) (actual time=0.001..0.002 rows=10 loops=10)               ->  ProjectSet  (cost=0.00..0.07 rows=10 width=32) (actual time=0.001..0.004 rows=10 loops=1)                     ->  Result  (cost=0.00..0.01 rows=1 width=0) (actual time=0.000..0.001 rows=1 loops=1) Planning Time: 180.482 ms Execution Time: 0.148 ms(9 rows)I am tempted to stop short of educating the planner about the possibility to re-scan (thus dropping the materialise node) during a join. It seems feasible, and sometimes advantageous. (Perhaps if the join quals would cause a huge amount of the output to be filtered??) But it also seems better to treat it as an entirely separate issue.cost_functionscan and cost_rescan would likely need some adjustment ifpossible.I looked at cost_functionscan(), but I think it is already of the view that a function can pipeline. It computes a startup_cost and a run_cost, where run_cost is the per-tuple cost * num_rows. With this understanding, it is actually wrong given the current materialisation-always behaviour. I think this means I don’t need to make any fundamental changes in order to correctly cost the new behaviour.However, I'm not sure that the planner has any way to knowwhether a given SRF will support ValuePer

Re: [HACKERS] Block level parallel vacuum

[Amit Kapila]

On Fri, Oct 4, 2019 at 7:57 PM Masahiko Sawada 
wrote:

> On Fri, Oct 4, 2019 at 2:31 PM Amit Kapila 
> wrote:
> >>
> >
> > Do we really need to log all those messages?  The other places where we
> launch parallel workers doesn't seem to be using such messages.  Why do you
> think it is important to log the messages here when other cases don't use
> it?
>
> Well I would rather think that parallel create index doesn't log
> enough messages. Parallel maintenance operation is invoked manually by
> user. I can imagine that DBA wants to cancel and try the operation
> again later if enough workers are not launched. But there is not a
> convenient way to confirm how many parallel workers planned and
> actually launched. We need to see ps command or pg_stat_activity.
> That's why I think that log message would be helpful for users.
>

Hmm, what is a guarantee at a later time the user will get the required
number of workers?  I think if the user decides to vacuum, then she would
want it to start sooner.  Also, to cancel the vacuum, for this reason, the
user needs to monitor logs which don't seem to be an easy thing considering
this information will be logged at DEBUG2 level.  I think it is better to
add in docs that we don't guarantee that the number of workers the user has
asked or expected to use for a parallel vacuum will be available during
execution.  Even if there is a compelling reason (which I don't see)  to
log this information, I think we shouldn't use more than one message to log
(like there is no need for a separate message for cleanup and vacuuming)
this information.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Non-null values of recovery functions after promote or crash of primary

[Martín Marqués]

Hi,

Yesterday we (that's me and my colleague Ricardo Gomez) were working on
an issue where a monitoring script was returning increasing lag
information on a primary instead of a NULL value.

The query used involved the following functions (the function was
amended to work-around the issue I'm reporting here):

pg_last_wal_receive_lsn()
pg_last_wal_replay_lsn()
pg_last_xact_replay_timestamp()

Under normal circumstances we would expect to receive NULLs from all
three functions on a primary node, and code comments back up my thoughts.

The problem is, what if the node is a standby which was promoted without
restarting, or that had to perform crash recovery?

So during the time it's recovering the values in ` XLogCtl` are updated
with recovery information, and once the recovery finishes, due to crash
recovery reaching a consistent state, or a promotion of a standby
happening, those values are not reset to startup defaults.

That's when you start seeing non-null values returned by
`pg_last_wal_replay_lsn()`and `pg_last_xact_replay_timestamp()`.

Now, I don't know if we should call this a bug, or an undocumented
anomaly. We could fix the bug by resetting the values from ` XLogCtl`
after finishing recovery, or document that we might see non-NULL values
in certain cases.

Regards,

-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Re: [PATCH] Do not use StdRdOptions in Access Methods

[Nikolay Shaplov]

В Fri, 27 Sep 2019 17:24:49 +0900
Michael Paquier  пишет:

> > Looks like some good actionable feedback.  I've moved this patch to
> > September, and set it to "Waiting on Author".
>
> The patch is in this state for two months now, so I have switched it
> to "returned with feedback".  The latest patch does not apply, and it
> would require an update for the new test module dummy_index_am.

I've been thinking about this patch and came to a conclusion that it
would be better to split it to even smaller parts, so they can be
easily reviewed, one by one. May be even leaving most complex
Heap/Toast part for later.

This is first part of this patch. Here we give each Access Method it's
own option binary representation instead of StdRdOptions.

I think this change is obvious. Because, first, Access Methods do not
use most of the values defined in StdRdOptions.

Second this patch make Options structure code unified for all core
Access Methods. Before some AM used StdRdOptions, some AM used it's own
structure, now all AM uses own structures and code is written in the
same style, so it would be more easy to update it in future.

John Dent, would you join reviewing this part of the patch? I hope it
will be more easy now...


-- 
Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da
Body-oriented Therapist: https://vk.com/nataraj_rebalancing  (Russian)diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index b5072c0..33f770b 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -23,7 +23,7 @@
 #include "access/htup_details.h"
 #include "access/nbtree.h"
 #include "access/reloptions.h"
-#include "access/spgist.h"
+#include "access/spgist_private.h"
 #include "catalog/pg_type.h"
 #include "commands/defrem.h"
 #include "commands/tablespace.h"
diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c
index 838ee68..eec2100 100644
--- a/src/backend/access/hash/hashpage.c
+++ b/src/backend/access/hash/hashpage.c
@@ -359,7 +359,7 @@ _hash_init(Relation rel, double num_tuples, ForkNumber forkNum)
 	data_width = sizeof(uint32);
 	item_width = MAXALIGN(sizeof(IndexTupleData)) + MAXALIGN(data_width) +
 		sizeof(ItemIdData);		/* include the line pointer */
-	ffactor = RelationGetTargetPageUsage(rel, HASH_DEFAULT_FILLFACTOR) / item_width;
+	ffactor = (BLCKSZ * HashGetFillFactor(rel) / 100) / item_width;
 	/* keep to a sane range */
 	if (ffactor < 10)
 		ffactor = 10;
diff --git a/src/backend/access/hash/hashutil.c b/src/backend/access/hash/hashutil.c
index 3fb92ea..5170634 100644
--- a/src/backend/access/hash/hashutil.c
+++ b/src/backend/access/hash/hashutil.c
@@ -289,7 +289,28 @@ _hash_checkpage(Relation rel, Buffer buf, int flags)
 bytea *
 hashoptions(Datum reloptions, bool validate)
 {
-	return default_reloptions(reloptions, validate, RELOPT_KIND_HASH);
+	relopt_value *options;
+	HashRelOptions *rdopts;
+	int			numoptions;
+	static const relopt_parse_elt tab[] = {
+		{"fillfactor", RELOPT_TYPE_INT, offsetof(HashRelOptions, fillfactor)},
+	};
+
+	options = parseRelOptions(reloptions, validate, RELOPT_KIND_HASH,
+			  &numoptions);
+
+	/* if none set, we're done */
+	if (numoptions == 0)
+		return NULL;
+
+	rdopts = allocateReloptStruct(sizeof(HashRelOptions), options, numoptions);
+
+	fillRelOptions((void *) rdopts, sizeof(HashRelOptions), options, numoptions,
+   validate, tab, lengthof(tab));
+
+	pfree(options);
+
+	return (bytea *) rdopts;
 }
 
 /*
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 4cfd528..b460d13 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -816,7 +816,7 @@ _bt_vacuum_needs_cleanup(IndexVacuumInfo *info)
 	}
 	else
 	{
-		StdRdOptions *relopts;
+		BTRelOptions *relopts;
 		float8		cleanup_scale_factor;
 		float8		prev_num_heap_tuples;
 
@@ -827,7 +827,7 @@ _bt_vacuum_needs_cleanup(IndexVacuumInfo *info)
 		 * tuples exceeds vacuum_cleanup_index_scale_factor fraction of
 		 * original tuples count.
 		 */
-		relopts = (StdRdOptions *) info->index->rd_options;
+		relopts = (BTRelOptions *) info->index->rd_options;
 		cleanup_scale_factor = (relopts &&
 relopts->vacuum_cleanup_index_scale_factor >= 0)
 			? relopts->vacuum_cleanup_index_scale_factor
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index ab19692..f7c2377 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -725,8 +725,9 @@ _bt_pagestate(BTWriteState *wstate, uint32 level)
 	if (level > 0)
 		state->btps_full = (BLCKSZ * (100 - BTREE_NONLEAF_FILLFACTOR) / 100);
 	else
-		state->btps_full = RelationGetTargetPageFreeSpace(wstate->index,
-		  BTREE_DEFAULT_FILLFACTOR);
+		state->btps_full = (BLCKSZ * (100 - BTGetFillFactor(wstate->index))
+			 / 100);
+
 	/* no parent level, yet */
 	state->btps_next = NULL;
 
diff --gi

Re: [HACKERS] Deadlock in XLogInsert at AIX

[Noah Misch]

On Sat, Aug 31, 2019 at 03:30:26PM -0700, Noah Misch wrote:
> On Sat, Aug 31, 2019 at 02:27:55PM -0400, Tom Lane wrote:
> > Noah Misch  writes:
> > > Done.  fetch-add-variable-test-v1.patch just adds tests for non-constant
> > > addends and 16-bit edge cases.  Today's implementation handles those,
> > > PostgreSQL doesn't use them, and I might easily have broken them.
> > > fetch-add-xlc-asm-v1.patch moves xlc builds from the __fetch_and_add()
> > > intrinsic to inline asm.  fetch-add-gcc-xlc-unify-v1.patch moves 
> > > fetch_add to
> > > inline asm for all other ppc compilers.  gcc-7.2.0 generates equivalent 
> > > code
> > > before and after.  I plan to keep the third patch HEAD-only, 
> > > back-patching the
> > > other two.  I tested with xlc v12 and v13.
> > 
> > Hm, no objection to the first two patches, but I don't understand
> > why the third patch goes to so much effort just to use "addi" rather
> > than (one assumes) "li" then "add"?  It doesn't seem likely that
> > that's buying much.
> 
> Changing an addi to li+add may not show up on benchmarks, but I can't claim
> it's immaterial.  I shouldn't unify the code if that makes the compiled code
> materially worse than what the gcc intrinsics produce today, hence the
> nontrivial (~50 line) bits to match the intrinsics' capabilities.

The first two patches have worked so far, but fetch-add-gcc-xlc-unify-v1.patch
broke older gcc: https://postgr.es/m/flat/7517.1568470...@sss.pgh.pa.us.  That
side thread settled on putting pg_atomic_compare_exchange_u{32,64}_impl
implementations in arch-ppc.h.  Attached fetch-add-gcc-xlc-unify-v2.patch does
so.  For compare_exchange, the generated code doesn't match gcc intrinsics
exactly; code comments discuss this.  Compared to v1, its other change is to
extract common __asm__ templates into macros instead of repeating them four
times (32/64bit, variable/const).
diff --git a/configure b/configure
index 7a6bfc2..d6ecb33 100755
--- a/configure
+++ b/configure
@@ -14650,6 +14650,46 @@ $as_echo "$pgac_cv_have_ppc_mutex_hint" >&6; }
 $as_echo "#define HAVE_PPC_LWARX_MUTEX_HINT 1" >>confdefs.h
 
 fi
+# Check if compiler accepts "i"(x) when __builtin_constant_p(x).
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether 
__builtin_constant_p(x) implies \"i\"(x) acceptance" >&5
+$as_echo_n "checking whether __builtin_constant_p(x) implies \"i\"(x) 
acceptance... " >&6; }
+if ${pgac_cv_have_i_constraint__builtin_constant_p+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+static inline int
+ addi(int ra, int si)
+ {
+ int res = 0;
+ if (__builtin_constant_p(si))
+ __asm__ __volatile__(
+ " addi %0,%1,%2\n" : "=r"(res) : "r"(ra), "i"(si));
+ return res;
+ }
+ int test_adds(int x) { return addi(3, x) + addi(x, 5); }
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  pgac_cv_have_i_constraint__builtin_constant_p=yes
+else
+  pgac_cv_have_i_constraint__builtin_constant_p=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: 
$pgac_cv_have_i_constraint__builtin_constant_p" >&5
+$as_echo "$pgac_cv_have_i_constraint__builtin_constant_p" >&6; }
+if test x"$pgac_cv_have_i_constraint__builtin_constant_p" = xyes ; then
+
+$as_echo "#define HAVE_I_CONSTRAINT__BUILTIN_CONSTANT_P 1" >>confdefs.h
+
+fi
   ;;
 esac
 
diff --git a/configure.in b/configure.in
index dde3eec..510bd76 100644
--- a/configure.in
+++ b/configure.in
@@ -1541,6 +1541,26 @@ case $host_cpu in
 if test x"$pgac_cv_have_ppc_mutex_hint" = xyes ; then
AC_DEFINE(HAVE_PPC_LWARX_MUTEX_HINT, 1, [Define to 1 if the assembler 
supports PPC's LWARX mutex hint bit.])
 fi
+# Check if compiler accepts "i"(x) when __builtin_constant_p(x).
+AC_CACHE_CHECK([whether __builtin_constant_p(x) implies "i"(x) acceptance],
+   [pgac_cv_have_i_constraint__builtin_constant_p],
+[AC_COMPILE_IFELSE([AC_LANG_PROGRAM(
+[static inline int
+ addi(int ra, int si)
+ {
+ int res = 0;
+ if (__builtin_constant_p(si))
+ __asm__ __volatile__(
+ " addi %0,%1,%2\n" : "=r"(res) : "r"(ra), "i"(si));
+ return res;
+ }
+ int test_adds(int x) { return addi(3, x) + addi(x, 5); }], [])],
+[pgac_cv_have_i_constraint__builtin_constant_p=yes],
+[pgac_cv_have_i_constraint__builtin_constant_p=no])])
+if test x"$pgac_cv_have_i_constraint__builtin_constant_p" = xyes ; then
+  AC_DEFINE(HAVE_I_CONSTRAINT__BUILTIN_CONSTANT_P, 1,
+[Define to 1 if __builtin_constant_p(x) implies "i"(x) 
acceptance.])
+fi
   ;;
 esac
 
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 512213a..9be84fc 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -332,6 +332,9 @@
 /* Defi

expressive test macros (was: Report test_atomic_ops() failures consistently, via macros)

[Andres Freund]

Hi,

On 2019-10-05 17:08:38 +, Noah Misch wrote:
> Report test_atomic_ops() failures consistently, via macros.
> 
> This prints the unexpected value in more failure cases, and it removes
> forty-eight hand-maintained error messages.  Back-patch to 9.5, which
> introduced these tests.

Thanks for these, that's a nice improvement.

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=639feb92e1186e1de495d1bfdb191869cb56450a

...
+#define EXPECT_EQ_U32(result_expr, expected_expr)  \
+   do { \
+   uint32  result = (result_expr); \
+   uint32  expected = (expected_expr); \
+   if (result != expected) \
+   elog(ERROR, \
+"%s yielded %u, expected %s in file \"%s\" line %u", \
+#result_expr, result, #expected_expr, __FILE__, __LINE__); \
+   } while (0)
...


I wonder if we should put these (and a few more, for other types), into
a more general place. I would like to have them for writing both tests
like regress.c:test_atomic_ops(), and for writing assertions that
actually display useful error messages.  For the former it makes sense
to ERROR out, for the latter they ought to abort, as currently.

Seems like putting ASSERT_{EQ,LT,...}_{U32,S32,...} (or Assert_Eq_...,
but that'd imo look weirder than the inconsistency) into c.h would make
sense, and EXPECT_ somewhere in common/pg_test.h or such?

Greetings,

Andres Freund

Re: Transparent Data Encryption (TDE) and encrypted files

[Tomas Vondra]

On Fri, Oct 04, 2019 at 08:14:44PM -0400, Bruce Momjian wrote:

On Sat, Oct  5, 2019 at 12:54:35AM +0200, Tomas Vondra wrote:

On Fri, Oct 04, 2019 at 06:06:10PM -0400, Bruce Momjian wrote:
> For full-cluster TDE with AES-NI-enabled, the performance impact is
> usually ~4%, so doing anything more granular doesn't seem useful.  See
> this PGCon presentation with charts:
>
>https://www.youtube.com/watch?v=TXKoo2SNMzk#t=27m50s
>
> Having anthing more fine-grained that all-cluster didn't seem worth it.
> Using per-user keys is useful, but also much harder to implement.
>

Not sure I follow. I thought you are asking why Oracle apparently does
not leverage AES-NI for column-level encryption (at least according to
the document I linked)? And I don't know why that's the case.


No, I read it as Oracle saying that there isn't much value to per-column
encryption if you have crypto hardware acceleration, because the
all-cluster encryption overhead is so minor.



So essentially the argument is - if you have hw crypto acceleration (aka
AES-NI), then the overhead of all-cluster encryption is so low it does
not make sense to bother with lowering it with column encryption.

IMO that's a good argument against column encryption (at least when used
to reduce overhead), although 10% still quite a bit.

But I'm not sure it's what the document is saying. I'm sure if they
could, they'd use AES-NI even for column encryption, to make it more
efficient. Because why wouldn't you do that? But the doc explicitly
says:

   Hardware cryptographic acceleration for TDE column encryption is
   not supported.

So there has to be a reason why that's not supported. Either there's
something that prevents this mode from using AES-NI at all, or it simply
can't be sped-up.


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services 

Re: How to install login_hook in Postgres 10.5

[legrand legrand]

pavan95 wrote
> Hello Community,
> 
> While I was searching for logon trigger in postgres similar to that of
> Oracle, I came across "login_hook", which can be installed as a  Postgres
> database extension to mimic a logon trigger.
> 
> But I tried to install but failed. Error is that it could not find its .so
> file.  Could you please help me in installing this login_hook ??
> 
> Looking forward to hear from you. Any suggestions would be of great help!
> 
> Thanks & Regards,
> Pavan
> 
> 
> 
> --
> Sent from:
> http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

issue
ERROR:  could not access file "login_hook.so": No such file or directory
has been fixed see:
https://github.com/splendiddata/login_hook/issues/1

Regards
PAscal



--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

[PATCH] Add some useful asserts into View Options macroses

[Nikolay Shaplov]

This thread is a follow up to the thread 
https://www.postgresql.org/message-id/2620882.s52SJui4ql@x200m where I've been 
trying to remove StdRdOptions 
structure and replace it with unique structure for each relation kind.

I've decided to split that patch into smaller parts.

This part adds some asserts to ViewOptions macroses.
Since an option pointer there is converted into (ViewOptions *) it would be 
really good to make sure that this macros is called in proper context, and we 
do the convertation properly. At least when running tests with asserts turned 
on.

-- 
Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da
Body-oriented Therapist: https://vk.com/nataraj_rebalancing  (Russian)diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index a5cf804..b24fb1d 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -351,9 +351,10 @@ typedef struct ViewOptions
  *		Returns whether the relation is security view, or not.  Note multiple
  *		eval of argument!
  */
-#define RelationIsSecurityView(relation)	\
-	((relation)->rd_options ?\
-	 ((ViewOptions *) (relation)->rd_options)->security_barrier : false)
+#define RelationIsSecurityView(relation)	\
+	(AssertMacro(relation->rd_rel->relkind == RELKIND_VIEW),\
+	 ((relation)->rd_options ?\
+	  ((ViewOptions *) (relation)->rd_options)->security_barrier : false))
 
 /*
  * RelationHasCheckOption
@@ -361,9 +362,10 @@ typedef struct ViewOptions
  *		or the cascaded check option.  Note multiple eval of argument!
  */
 #define RelationHasCheckOption(relation)	\
+	(AssertMacro(relation->rd_rel->relkind == RELKIND_VIEW),\
 	((relation)->rd_options &&\
 	 ((ViewOptions *) (relation)->rd_options)->check_option !=\
-	 VIEW_OPTION_CHECK_OPTION_NOT_SET)
+	 VIEW_OPTION_CHECK_OPTION_NOT_SET))
 
 /*
  * RelationHasLocalCheckOption
@@ -371,9 +373,10 @@ typedef struct ViewOptions
  *		option.  Note multiple eval of argument!
  */
 #define RelationHasLocalCheckOption(relation)\
+	(AssertMacro(relation->rd_rel->relkind == RELKIND_VIEW),\
 	((relation)->rd_options &&\
 	 ((ViewOptions *) (relation)->rd_options)->check_option ==\
-	 VIEW_OPTION_CHECK_OPTION_LOCAL)
+	 VIEW_OPTION_CHECK_OPTION_LOCAL))
 
 /*
  * RelationHasCascadedCheckOption
@@ -381,9 +384,10 @@ typedef struct ViewOptions
  *		option.  Note multiple eval of argument!
  */
 #define RelationHasCascadedCheckOption(relation)			\
+	(AssertMacro(relation->rd_rel->relkind == RELKIND_VIEW),\
 	((relation)->rd_options &&\
 	 ((ViewOptions *) (relation)->rd_options)->check_option ==\
-	  VIEW_OPTION_CHECK_OPTION_CASCADED)
+	  VIEW_OPTION_CHECK_OPTION_CASCADED))
 
 /*
  * RelationIsValid


signature.asc
Description: This is a digitally signed message part.

Re: New "-b slim" option in 2019b zic: should we turn that on?

[Andrew Dunstan]

On 7/17/19 6:42 PM, Tom Lane wrote:
>
> Despite the marginal payoff, I'm strongly tempted to enable this
> switch.  The only reason I can think of not to do it is if somebody
> is using a Postgres installation's share/timezone tree as tzdata
> for some other program with not-up-to-date timezone library code.
> But who would that be?
>
> A possible compromise is to turn it on only in HEAD, though I'd
> rather keep all the branches working the same as far as the
> timezone code goes.
>


I've just run into an issue with this (commit a1207910968). The makefile
now assumes that zic has this switch. But I was attempting to get around
an issue on msys2 by using its zic, (ZIC=/usr/bin/zic configure ...). It
crashes on the floor because it doesn't know about "-b slim". I think we
probably need a way to turn this off.


cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: New "-b slim" option in 2019b zic: should we turn that on?

[Tom Lane]

Andrew Dunstan  writes:
> I've just run into an issue with this (commit a1207910968). The makefile
> now assumes that zic has this switch. But I was attempting to get around
> an issue on msys2 by using its zic, (ZIC=/usr/bin/zic configure ...). It
> crashes on the floor because it doesn't know about "-b slim". I think we
> probably need a way to turn this off.

I had contemplated injecting the -b switch via

 # any custom options you might want to pass to zic while installing data files
-ZIC_OPTIONS =
+ZIC_OPTIONS = -b slim

which would allow overriding it by defining the ZIC_OPTIONS macro.
Does that seem appropriate?  I didn't do it because I worried about
interference with existing uses of ZIC_OPTIONS ... but who knows
whether there are any.

BTW, building with old versions of zic is not guaranteed to work anyway.
They do tend to wait a year or two before they start to use new zic
features in the timezone data files, but they don't wait indefinitely.

regards, tom lane

Re: expressive test macros (was: Report test_atomic_ops() failures consistently, via macros)

[Noah Misch]

On Sat, Oct 05, 2019 at 12:07:29PM -0700, Andres Freund wrote:
> +#define EXPECT_EQ_U32(result_expr, expected_expr)  \
> +   do { \
> +   uint32  result = (result_expr); \
> +   uint32  expected = (expected_expr); \
> +   if (result != expected) \
> +   elog(ERROR, \
> +"%s yielded %u, expected %s in file \"%s\" line %u", \
> +#result_expr, result, #expected_expr, __FILE__, __LINE__); \
> +   } while (0)
> ...
> 
> 
> I wonder if we should put these (and a few more, for other types), into
> a more general place. I would like to have them for writing both tests
> like regress.c:test_atomic_ops(), and for writing assertions that
> actually display useful error messages.  For the former it makes sense
> to ERROR out, for the latter they ought to abort, as currently.
> 
> Seems like putting ASSERT_{EQ,LT,...}_{U32,S32,...} (or Assert_Eq_...,
> but that'd imo look weirder than the inconsistency) into c.h would make
> sense, and EXPECT_ somewhere in common/pg_test.h or such?

Sounds reasonable.  For broader use, I would include the expected value, not
just expected_expr:

 elog(ERROR, \
  "%s yielded %u, expected %s (%u) in file \"%s\" line %u", \
  #result_expr, result, #expected_expr, expected, __FILE__, __LINE__); \

I didn't do that for the atomics tests, where expected_expr is always trivial.
The codebase has plenty of Assert(x == y) where either of x or y could have
the surprising value.

Re: New "-b slim" option in 2019b zic: should we turn that on?

[Andrew Dunstan]

On 10/5/19 6:33 PM, Tom Lane wrote:
> Andrew Dunstan  writes:
>> I've just run into an issue with this (commit a1207910968). The makefile
>> now assumes that zic has this switch. But I was attempting to get around
>> an issue on msys2 by using its zic, (ZIC=/usr/bin/zic configure ...). It
>> crashes on the floor because it doesn't know about "-b slim". I think we
>> probably need a way to turn this off.
> I had contemplated injecting the -b switch via
>
>  # any custom options you might want to pass to zic while installing data 
> files
> -ZIC_OPTIONS =
> +ZIC_OPTIONS = -b slim
>
> which would allow overriding it by defining the ZIC_OPTIONS macro.
> Does that seem appropriate?  I didn't do it because I worried about
> interference with existing uses of ZIC_OPTIONS ... but who knows
> whether there are any.


I don't think that's going to work very well with a buildfarm member,
where there's no convenient way to set it.


But it turns out there are bigger problems with what I'm doing, anyway,
so let's leave sleeping dogs lie for now.


cheers


andrew


-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: New "-b slim" option in 2019b zic: should we turn that on?

[Tom Lane]

Andrew Dunstan  writes:
> On 10/5/19 6:33 PM, Tom Lane wrote:
>> I had contemplated injecting the -b switch via
>> -ZIC_OPTIONS =
>> +ZIC_OPTIONS = -b slim

> I don't think that's going to work very well with a buildfarm member,
> where there's no convenient way to set it.

Can't you set that from build_env?

regards, tom lane

Non-Active links being referred in our source code

[vignesh C]

Hi,

There are few links present in our source files for which the web links are no 
more active.
Details for the same is given below:

Sl No
Link
Referred File
1
http://h21007.www2.hp.com/portal/download/files/unprot/Itanium/inline_assem_ERS.pdf
 

   
generic-acc.h
2
http://h21007.www2.hp.com/portal/download/files/unprot/itanium/spinlocks.pdf 
  
   
generic-acc.h
3
http://www.comp.nus.edu.sg/~wuyongzh/my_doc/ntstatus.txt 
 
win32_port.h
4
http://www.microsoft.com/msj/0197/exception/exception.aspx 
 
win32_port.h
5
http://www.nologs.com/ntstatus.html    
win32_port.h
6
http://www.ross.net/crc/  
pg_crc.h
7
http://www.ross.net/crc/download/crc_v3.txt 
 
pg_crc.h
8
http://www.codeproject.com/string/dmetaphone1.asp 
   
dmetaphone.c
9
http://www.postgresql.org/2009/explain  
explain.c
10
http://www.merriampark.com/ld.htm    
levenshtein.c
11
http://www.cs.auckland.ac.nz/software/AlgAnim/niemann/s_man.htm 
 
rbtree.c

I could not find the equivalent links for the same.
Should we update the links for the same?


Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com