Re: proposal - log_full_scan

[Pavel Stehule]

so 17. 4. 2021 v 20:51 odesílatel Pavel Stehule 
napsal:

>
>
> so 17. 4. 2021 v 20:36 odesílatel Justin Pryzby 
> napsal:
>
>> On Sat, Apr 17, 2021 at 05:22:59PM +0200, Pavel Stehule wrote:
>> > so 17. 4. 2021 v 17:09 odesílatel Justin Pryzby 
>> napsal:
>> >
>> > > On Sat, Apr 17, 2021 at 04:36:52PM +0200, Pavel Stehule wrote:
>> > > > today I worked on postgres's server used for critical service.
>> Because the
>> > > > application is very specific, we had to do final tuning on
>> production
>> > > > server. I fix lot of queries, but I am not able to detect fast
>> queries that
>> > > > does full scan of middle size tables - to 1M rows. Surely I
>> wouldn't log
>> > > > all queries. Now, there are these queries with freq 10 per sec.
>> > > >
>> > > > Can be nice to have a possibility to set a log of  queries that do
>> full
>> > > > scan and read more tuples than is specified limit or that does full
>> scan of
>> > > > specified tables.
>> > > >
>> > > > What do you think about the proposed feature?
>> > >
>> > > Are you able to use auto_explain with auto_explain.log_min_duration ?
>> >
>> > Unfortunately,  I cannot use it. This server executes 5K queries per
>> > seconds, and I am afraid to decrease log_min_duration.
>> >
>> > The logs are forwarded to the network and last time, when users played
>> with
>> > it, then they had problems with the network.
>> ..
>> > The fullscan of this table needs about 30ms and has 200K rows. So
>> > decreasing log_min_duration to this value is very risky.
>>
>> auto_explain.sample_rate should allow setting a sufficiently low value of
>> log_min_duration.  It exists since v9.6.
>>
>>
> It cannot help - these queries are executed a few times per sec. In same
> time this server execute 500 - 1000 other queries per sec
>

maybe this new option for server and for auto_explain can be just simple

log_seqscan = (minimum number of tuples from one relation)
auto_explain.log_seqscan = (minimum number of tuples from one relation)

This is a similar feature like log_temp_files. Next step can be
implementing this feature like a table option.

What do you think about it?

Regards

Pavel

The extension like pg_qualstat is good, but it does different work. In
complex applications I need to detect buggy (forgotten) queries - last week
I found two queries over bigger tables without predicates. So the qualstat
doesn't help me. This is an application for a government with few (but for
government typical) specific: 1) the life cycle is short (one month), 2)
there is not slow start - from first moment the application will be used by
more hundred thousands people, 3) the application is very public - so any
issues are very interesting for press and very unpleasant for politics, and
in next step for all suppliers (there are high penalty for failures), and
an admins are not happy from external extensions, 4) the budget is not too
big - there is not any performance testing environment

First stages are covered well today. We can log and process very slow
queries, and fix it immediately - with CREATE INDEX CONCURRENTLY I can do
it well on production servers too without high risk.

But the detection of some bad not too slow queries is hard. And as an
external consultant I am not able to install any external extensions to the
production environment for fixing some hot issues, The risk is not
acceptable for project managers and I understand. So I have to use only
tools available in Postgres.




> Regards
>
> Pavel
>
>
>> --
>> Justin
>>
>

Re: Replication slot stats misgivings

[vignesh C]

On Sun, Apr 18, 2021 at 8:43 AM Amit Kapila  wrote:
>
> On Sun, Apr 18, 2021 at 7:36 AM vignesh C  wrote:
> >
> > On Sun, Apr 18, 2021 at 3:51 AM Tom Lane  wrote:
> > >
> > > I wrote:
> > > > The buildfarm suggests that this isn't entirely stable:
> > > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=anole=2021-04-17%2011%3A14%3A49
> > > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bichir=2021-04-17%2016%3A30%3A15
> > >
> > > Oh, I missed that hyrax is showing the identical symptom:
> > >
> > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hyrax=2021-04-16%2007%3A05%3A44
> > >
> > > So you might try CLOBBER_CACHE_ALWAYS to see if you can reproduce it
> > > that way.
> > >
> >
> > I will try to check and identify why it is failing.
> >
>
> I think the failure is due to the reason that in the new tests after
> reset, we are not waiting for the stats message to be delivered as we
> were doing in other cases. Also, for the new test (non-spilled case),
> we need to decode changes as we are doing for other tests, otherwise,
> it will show the old stats.

I also felt that is the reason for the failure, I will fix and post a
patch for this.

Regards,
Vignesh

Re: Replication slot stats misgivings

[Amit Kapila]

On Sun, Apr 18, 2021 at 7:36 AM vignesh C  wrote:
>
> On Sun, Apr 18, 2021 at 3:51 AM Tom Lane  wrote:
> >
> > I wrote:
> > > The buildfarm suggests that this isn't entirely stable:
> > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=anole=2021-04-17%2011%3A14%3A49
> > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bichir=2021-04-17%2016%3A30%3A15
> >
> > Oh, I missed that hyrax is showing the identical symptom:
> >
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hyrax=2021-04-16%2007%3A05%3A44
> >
> > So you might try CLOBBER_CACHE_ALWAYS to see if you can reproduce it
> > that way.
> >
>
> I will try to check and identify why it is failing.
>

I think the failure is due to the reason that in the new tests after
reset, we are not waiting for the stats message to be delivered as we
were doing in other cases. Also, for the new test (non-spilled case),
we need to decode changes as we are doing for other tests, otherwise,
it will show the old stats.

-- 
With Regards,
Amit Kapila.

Re: Replication slot stats misgivings

[vignesh C]

On Sun, Apr 18, 2021 at 3:51 AM Tom Lane  wrote:
>
> I wrote:
> > The buildfarm suggests that this isn't entirely stable:
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=anole=2021-04-17%2011%3A14%3A49
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bichir=2021-04-17%2016%3A30%3A15
>
> Oh, I missed that hyrax is showing the identical symptom:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hyrax=2021-04-16%2007%3A05%3A44
>
> So you might try CLOBBER_CACHE_ALWAYS to see if you can reproduce it
> that way.
>

I will try to check and identify why it is failing.

Regards,
Vignesh

Re: terminate called after throwing an instance of 'std::bad_alloc'

[Justin Pryzby]

On Fri, Apr 16, 2021 at 10:18:37PM -0500, Justin Pryzby wrote:
> On Fri, Apr 16, 2021 at 09:48:54PM -0500, Justin Pryzby wrote:
> > On Fri, Apr 16, 2021 at 07:17:55PM -0700, Andres Freund wrote:
> > > Hi,
> > > 
> > > On 2020-12-18 17:56:07 -0600, Justin Pryzby wrote:
> > > > I'd be happy to run with a prototype fix for the leak to see if the 
> > > > other issue
> > > > does (not) recur.
> > > 
> > > I just posted a prototype fix to 
> > > https://www.postgresql.org/message-id/20210417021602.7dilihkdc7oblrf7%40alap3.anarazel.de
> > > (just because that was the first thread I re-found). It'd be cool if you
> > > could have a look!
> > 
> > This doesn't seem to address the problem triggered by the reproducer at
> > https://www.postgresql.org/message-id/20210331040751.gu4...@telsasoft.com
> > (sorry I didn't CC you)
> 
> I take that back - I forgot that this doesn't release RAM until hitting a
> threshold.

I tried this on the query that was causing the original c++ exception.

It still grows to 2GB within 5min.

  PID USER  PR  NIVIRTRESSHR S  %CPU %MEM TIME+ COMMAND 

  
23084 postgres  20   0 2514364   1.6g  29484 R  99.7 18.2   3:40.87 postgres: 
telsasoft ts 192.168.122.11(50892) SELECT   


  PID USER  PR  NIVIRTRESSHR S  %CPU %MEM TIME+ COMMAND 

  
23084 postgres  20   0 3046960   2.1g  29484 R 100.0 24.1   4:30.64 postgres: 
telsasoft ts 192.168.122.11(50892) SELECT   


  PID USER  PR  NIVIRTRESSHR S  %CPU %MEM TIME+ COMMAND 

  
23084 postgres  20   0 4323500   3.3g  29488 R  99.7 38.4   8:20.63 postgres: 
telsasoft ts 192.168.122.11(50892) SELECT   


When I first reported this issue, the affected process was a long-running,
single-threaded python tool.  We since updated it (partially to avoid issues
like this) to use multiprocessing, therefor separate postgres backends.

I'm now realizing that that's RAM use for a single query, not from continuous
leaks across multiple queries.  This is still true with the patch even if I
#define LLVMJIT_LLVM_CONTEXT_REUSE_MAX 1

  PID USER  PR  NIVIRTRESSHR S  %CPU %MEM TIME+ COMMAND 

  
28438 postgres  20   0 3854264   2.8g  29428 R  98.7 33.2   8:56.79 postgres: 
telsasoft ts 192.168.122.11(53614) BIND 


python3 ./jitleak.py # runs telsasoft reports
INFO:  recreating LLVM context after 2 uses
INFO:  recreating LLVM context after 2 uses
INFO:  recreating LLVM context after 2 uses
INFO:  recreating LLVM context after 2 uses
INFO:  recreating LLVM context after 2 uses
PID 27742 finished running report; est=None rows=40745; cols=34; ... 
duration:538
INFO:  recreating LLVM context after 81492 uses

I did:

-   llvm_llvm_context_reuse_count = 0;
Assert(llvm_context != NULL);
+   elog(INFO, "recreating LLVM context after %zu uses", 
llvm_llvm_context_reuse_count);
+   llvm_llvm_context_reuse_count = 0;

Maybe we're missing this condition somehow ?
if (llvm_jit_context_in_use_count == 0 &&

Also, I just hit this assertion by cancelling the query with ^C / sigint.  But
I don't have a reprodcer for it.

< 2021-04-17 19:14:23.509 ADT telsasoft >PANIC:  LLVMJitContext in use count 
not 0 at exit (is 1)

-- 
Justin

Re: Replication slot stats misgivings

[Tom Lane]

I wrote:
> The buildfarm suggests that this isn't entirely stable:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=anole=2021-04-17%2011%3A14%3A49
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bichir=2021-04-17%2016%3A30%3A15

Oh, I missed that hyrax is showing the identical symptom:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hyrax=2021-04-16%2007%3A05%3A44

So you might try CLOBBER_CACHE_ALWAYS to see if you can reproduce it
that way.

regards, tom lane

Re: Replication slot stats misgivings

[Tom Lane]

Amit Kapila  writes:
> I have pushed the first patch.

The buildfarm suggests that this isn't entirely stable:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=anole=2021-04-17%2011%3A14%3A49
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bichir=2021-04-17%2016%3A30%3A15

Each of those animals has also passed at least once since this went in,
so I'm betting on a timing-dependent issue.

regards, tom lane

Re: feature request ctid cast / sql exception

[David G. Johnston]

On Saturday, April 17, 2021, Tom Lane  wrote:

> "David G. Johnston"  writes:
> > On Sat, Apr 17, 2021 at 10:58 AM Vladimír Houba ml. 
> > wrote:
> >> Another nice feature would be a function that can be called from a sql
> >> statement and would throw an exception when executed.
>
> > An assertion-related extension in core would be welcomed.
>
> This has been suggested before, but as soon as you start looking
> at the details you find that it's really hard to get a one-size-fits-all
> definition that's any simpler than the existing plpgsql RAISE
> functionality.
>

Even just getting raise functionality as a standard functional api would be
a win.  I don’t imagine enough users would care enough to write their own
routines if one already existed, even if they would argue details about how
to create it in the first place.  For the expected use case of basically
developer-oriented error messages there is generally a acceptance of taking
the sufficient solution.

David J.

Re: feature request ctid cast / sql exception

[David G. Johnston]

On Sat, Apr 17, 2021 at 12:58 PM Vladimír Houba ml. 
wrote:

> I use ctid as a row identifier within a transaction in a Java application.
>

This doesn't present a very compelling argument since an actual user
declared primary key is what is expected to be used as a row identifier.
And as those are typically bigint if you follow this norm you get exactly
what you say you need.

David J.

Re: feature request ctid cast / sql exception

[Tom Lane]

"David G. Johnston"  writes:
> On Sat, Apr 17, 2021 at 10:58 AM Vladimír Houba ml. 
> wrote:
>> Another nice feature would be a function that can be called from a sql
>> statement and would throw an exception when executed.

> An assertion-related extension in core would be welcomed.

This has been suggested before, but as soon as you start looking
at the details you find that it's really hard to get a one-size-fits-all
definition that's any simpler than the existing plpgsql RAISE
functionality.  Different people have different ideas about how
much decoration they want around the message.  So, if 10% of the
world agrees with your choices and the other 90% keeps on using
a custom plpgsql function to do it their way, you haven't really
improved matters much.  OTOH a 90% solution might be interesting to
incorporate in core, but nobody's demonstrated that one exists.

regards, tom lane

Re: pg_amcheck option to install extension

[Mark Dilger]

> On Apr 16, 2021, at 11:06 AM, Andrew Dunstan  wrote:
> 
> 
> Hi,
> 
> Peter Geoghegan suggested that I have the cross version upgrade checker
> run pg_amcheck on the upgraded module. This seemed to me like a good
> idea, so I tried it, only to find that it refuses to run unless the
> amcheck extension is installed. That's fair enough, but it also seems to
> me like we should have an option to have pg_amcheck install the
> extension is it's not present, by running something like 'create
> extension if not exists amcheck'. Maybe in such a case there could also
> be an option to drop the extension when pg_amcheck's work is done - I
> haven't thought through all the implications.
> 
> Given pg_amcheck is a new piece of work I'm not sure if we can sneak
> this in under the wire for release 14. I will certainly undertake to
> review anything expeditiously. I can work around this issue in the
> buildfarm, but it seems like something other users are likely to want.

We cannot quite use a "create extension if not exists amcheck" command, as we 
clear the search path and so must specify the schema to use.  Should we instead 
avoid clearing the search path for this?  What are the security implications of 
using the first schema of the search path?

When called as `pg_amcheck --install-missing`, the implementation in the 
attached patch runs per database being checked a "create extension if not 
exists amcheck with schema public".  If called as `pg_amcheck 
--install-missing=foo`, it instead runs "create extension if not exists amcheck 
with schema foo` having escaped "foo" appropriately for the given database.  
There is no option to use different schemas for different databases.  Nor is 
there any option to use the search path.  If somebody needs that, I think they 
can manage installing amcheck themselves.

Does this meet your needs for v14?  I'd like to get this nailed down quickly, 
as it is unclear to me that we should even be doing this so late in the 
development cycle.

I'd also like your impressions on whether we're likely to move contrib/amcheck 
into core anytime soon.  If so, is it worth adding an option that we'll soon 
need to deprecate?



v1-0001-Adding-install-missing-option-to-pg_amcheck.patch
Description: Binary data


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: "could not find pathkey item to sort" for TPC-DS queries 94-96

[Tom Lane]

[ sorry for not getting to this thread till now ]

Tomas Vondra  writes:
> 3) Shouldn't find_em_expr_usable_for_sorting_rel now mostly mimic what
> prepare_sort_from_pathkeys does? That is, try to match the entries
> directly first, before the new pull_vars() business?

Yeah.  I concur that the problem here is that
find_em_expr_usable_for_sorting_rel isn't fully accounting for what
prepare_sort_from_pathkeys can and can't do.  However, I don't like this
patch much:

* As written, I think it may just move the pain somewhere else.  The point
of the logic in prepare_sort_from_pathkeys is to handle either full
expression matches (e.g. sort by "A+B" when "A+B" is an expression in
the input tlist) or computable expressions (sort by "A+B" when A and B
are individually available).  I think you've fixed the second case and
broken the first one.  Now it's possible that the case never arises,
and certainly failing to generate an early sort isn't catastrophic anyway.
But we ought to get it right.

* If the goal is to match what prepare_sort_from_pathkeys can do, I
think that doubling down on the strategy of having a duplicate copy
is not the path to a maintainable fix.

I think it's time for some refactoring of this code so that we can
actually share the logic.  Accordingly, I propose the attached.
It's really not that hard to share, as long as you accept the idea
that the list passed to the shared subroutine can be either a list of
TargetEntries or of bare expressions.

Also, I don't much care for either the name or API of
find_em_expr_usable_for_sorting_rel.  The sole current caller only
really needs a boolean result, and if it did need more than that
it'd likely need the whole EquivalenceMember not just the em_expr
(certainly createplan.c does).  So 0002 attached is some bikeshedding
on that.  I kept that separate because it might be wise to do it only
in HEAD, just in case somebody out there is calling the function from
an extension.

(BTW, responding to an upthread question: I think the looping to
remove multiple levels of RelabelType is probably now redundant,
but I didn't remove it.  If we want to do that there are more
places to touch than just this code.)

regards, tom lane

diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index 0188c1e9a1..5f7e505950 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -35,6 +35,7 @@
 static EquivalenceMember *add_eq_member(EquivalenceClass *ec,
 		Expr *expr, Relids relids, Relids nullable_relids,
 		bool is_child, Oid datatype);
+static bool exprlist_member_ignore_relabel(Expr *node, List *exprs);
 static void generate_base_implied_equalities_const(PlannerInfo *root,
    EquivalenceClass *ec);
 static void generate_base_implied_equalities_no_const(PlannerInfo *root,
@@ -769,6 +770,149 @@ get_eclass_for_sort_expr(PlannerInfo *root,
 	return newec;
 }
 
+/*
+ * find_ec_member_matching_expr
+ *		Locate an EquivalenceClass member matching the given expr, if any;
+ *		return NULL if no match.
+ *
+ * "Matching" is defined as "equal after stripping RelabelTypes".
+ * This is used for identifying sort expressions, and we need to allow
+ * binary-compatible relabeling for some cases involving binary-compatible
+ * sort operators.
+ *
+ * Child EC members are ignored unless they belong to given 'relids'.
+ */
+EquivalenceMember *
+find_ec_member_matching_expr(EquivalenceClass *ec,
+			 Expr *expr,
+			 Relids relids)
+{
+	ListCell   *lc;
+
+	/* We ignore binary-compatible relabeling on both ends */
+	while (expr && IsA(expr, RelabelType))
+		expr = ((RelabelType *) expr)->arg;
+
+	foreach(lc, ec->ec_members)
+	{
+		EquivalenceMember *em = (EquivalenceMember *) lfirst(lc);
+		Expr	   *emexpr;
+
+		/*
+		 * We shouldn't be trying to sort by an equivalence class that
+		 * contains a constant, so no need to consider such cases any further.
+		 */
+		if (em->em_is_const)
+			continue;
+
+		/*
+		 * Ignore child members unless they belong to the requested rel.
+		 */
+		if (em->em_is_child &&
+			!bms_is_subset(em->em_relids, relids))
+			continue;
+
+		/* Match if same expression (after stripping relabel) */
+		emexpr = em->em_expr;
+		while (emexpr && IsA(emexpr, RelabelType))
+			emexpr = ((RelabelType *) emexpr)->arg;
+
+		if (equal(emexpr, expr))
+			return em;
+	}
+
+	return NULL;
+}
+
+/*
+ * find_computable_ec_member
+ *		Locate an EquivalenceClass member that can be computed from the
+ *		expressions appearing in "exprs"; return NULL if no match.
+ *
+ * "exprs" can be either a list of bare expression trees, or a list of
+ * TargetEntry nodes.  Either way, it should contain Vars and possibly
+ * Aggrefs and WindowFuncs, which are matched to the corresponding elements
+ * of the EquivalenceClass's expressions.  As in find_ec_member_matching_expr,
+ * we ignore binary-compatible relabeling.
+ *
+ * Child EC members are ignored 

Re: feature request ctid cast / sql exception

[David G. Johnston]

On Sat, Apr 17, 2021 at 10:58 AM Vladimír Houba ml. 
wrote:

> I propose to implement a builtin and efficient bidirectional cast between
> ctid and bigint types.
>
>
Why?



> Another nice feature would be a function that can be called from a sql
> statement and would throw an exception when executed.
>
>
An assertion-related extension in core would be welcomed.

David J.

Re: proposal - log_full_scan

[Pavel Stehule]

so 17. 4. 2021 v 20:36 odesílatel Justin Pryzby 
napsal:

> On Sat, Apr 17, 2021 at 05:22:59PM +0200, Pavel Stehule wrote:
> > so 17. 4. 2021 v 17:09 odesílatel Justin Pryzby 
> napsal:
> >
> > > On Sat, Apr 17, 2021 at 04:36:52PM +0200, Pavel Stehule wrote:
> > > > today I worked on postgres's server used for critical service.
> Because the
> > > > application is very specific, we had to do final tuning on production
> > > > server. I fix lot of queries, but I am not able to detect fast
> queries that
> > > > does full scan of middle size tables - to 1M rows. Surely I wouldn't
> log
> > > > all queries. Now, there are these queries with freq 10 per sec.
> > > >
> > > > Can be nice to have a possibility to set a log of  queries that do
> full
> > > > scan and read more tuples than is specified limit or that does full
> scan of
> > > > specified tables.
> > > >
> > > > What do you think about the proposed feature?
> > >
> > > Are you able to use auto_explain with auto_explain.log_min_duration ?
> >
> > Unfortunately,  I cannot use it. This server executes 5K queries per
> > seconds, and I am afraid to decrease log_min_duration.
> >
> > The logs are forwarded to the network and last time, when users played
> with
> > it, then they had problems with the network.
> ..
> > The fullscan of this table needs about 30ms and has 200K rows. So
> > decreasing log_min_duration to this value is very risky.
>
> auto_explain.sample_rate should allow setting a sufficiently low value of
> log_min_duration.  It exists since v9.6.
>
>
It cannot help - these queries are executed a few times per sec. In same
time this server execute 500 - 1000 other queries per sec

Regards

Pavel


> --
> Justin
>

Re: proposal - log_full_scan

[Justin Pryzby]

On Sat, Apr 17, 2021 at 05:22:59PM +0200, Pavel Stehule wrote:
> so 17. 4. 2021 v 17:09 odesílatel Justin Pryzby  napsal:
> 
> > On Sat, Apr 17, 2021 at 04:36:52PM +0200, Pavel Stehule wrote:
> > > today I worked on postgres's server used for critical service. Because the
> > > application is very specific, we had to do final tuning on production
> > > server. I fix lot of queries, but I am not able to detect fast queries 
> > > that
> > > does full scan of middle size tables - to 1M rows. Surely I wouldn't log
> > > all queries. Now, there are these queries with freq 10 per sec.
> > >
> > > Can be nice to have a possibility to set a log of  queries that do full
> > > scan and read more tuples than is specified limit or that does full scan 
> > > of
> > > specified tables.
> > >
> > > What do you think about the proposed feature?
> >
> > Are you able to use auto_explain with auto_explain.log_min_duration ?
> 
> Unfortunately,  I cannot use it. This server executes 5K queries per
> seconds, and I am afraid to decrease log_min_duration.
> 
> The logs are forwarded to the network and last time, when users played with
> it, then they had problems with the network.
..
> The fullscan of this table needs about 30ms and has 200K rows. So
> decreasing log_min_duration to this value is very risky.

auto_explain.sample_rate should allow setting a sufficiently low value of
log_min_duration.  It exists since v9.6.

-- 
Justin

More info on pg_stat_activity Wait Event Name when is DataFileRead

[PegoraroF10]

I have a Publication/Subscription. Then 10 days ago my replication just
stopped, no one interesting message log on both sides. Then select from
pg_stat_replication was empty and some time later, an hour or two,
pg_stat_replication came back showing a record like it should. Well, it´s
replicating ? No, no one new record on replica, some more time and
pg_stat_replication was empty again. The only log was created was on
primary, initially wal_sender_timeout was 15 minutes. I tried changing it to
1 hour, maybe is a huge transaction, but no, the problem remains the same.
When I set timeout to 0 on Sender and Receiver, then I see that on replica
pg_stat_activity was DataFileRead on Wait Event Name field and IO on Wait
Event Type field, and that status remains the same for hours. Documentation
says DataFileRead is Waiting for a read from a relation data file, so it
does not specify which file is being waited. So, there is a problem on some
table on my replica but ... what file is this ? Well, it´ll spend a lot of
time but lets do a vacuum full. Then it took for 1 or 2 days but found two
corrupted tables. Those tables I couldn´t do select or dump, so I just
droped them and then finally replication came back.

This long explaining was only to show, at least for me, that would be
desirable to have an additional information when Postgres is waiting for a
file. What if DataFileRead showing relfilenode it´s waiting for ?

These logs are on primary, on replica I didn´t find any. And strangely,
those errors did not occur every wal_sender_timeout time, even when was
configured with 15 minutes they occured 2 or 3 times a day only.

log_time;user_name;session_start_time;error_severity;message
2021-04-06 16:21:23.385;replicate;2021-04-06 16:21:23.000;LOG;starting
logical decoding for slot sub_google_bkp
2021-04-06 16:21:23.386;replicate;2021-04-06 16:21:23.000;LOG;logical
decoding found consistent point at 5C1/5C7A3D48
2021-04-06 16:36:24.175;replicate;2021-04-06 16:21:23.000;LOG;terminating
walsender process due to replication timeout

2021-04-06 17:47:15.744;replicate;2021-04-06 17:47:15.000;LOG;starting
logical decoding for slot sub_google_bkp
2021-04-06 17:47:15.745;replicate;2021-04-06 17:47:15.000;LOG;logical
decoding found consistent point at 5C1/5CBFBF38
2021-04-06 18:02:15.757;replicate;2021-04-06 17:47:15.000;LOG;terminating
walsender process due to replication timeout

2021-04-07 11:59:27.831;replicate;2021-04-07 11:59:27.000;LOG;starting
logical decoding for slot sub_google_bkp
2021-04-07 11:59:27.832;replicate;2021-04-07 11:59:27.000;LOG;logical
decoding found consistent point at 5C1/5CBFBF38
2021-04-07 12:14:27.867;replicate;2021-04-07 11:59:27.000;LOG;terminating
walsender process due to replication timeout

2021-04-07 21:45:22.230;replicate;2021-04-07 21:45:22.000;LOG;starting
logical decoding for slot sub_google_bkp
2021-04-07 21:45:22.231;replicate;2021-04-07 21:45:22.000;LOG;logical
decoding found consistent point at 5C1/5CBFD438
2021-04-07 22:45:22.586;replicate;2021-04-07 21:45:22.000;LOG;terminating
walsender process due to replication timeout

2021-04-08 00:06:26.253;replicate;2021-04-08 00:06:25.000;LOG;starting
logical decoding for slot sub_google_bkp
2021-04-08 00:06:26.255;replicate;2021-04-08 00:06:25.000;LOG;logical
decoding found consistent point at 5C1/5CCF7E20
2021-04-08 02:15:10.342;replicate;2021-04-08 00:06:25.000;LOG;terminating
walsender process due to replication timeout




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

feature request ctid cast / sql exception

[Vladimír Houba ml .]

Hello,

this is a feature request for a rather simple functionality.

I propose to implement a builtin and efficient bidirectional cast between
ctid and bigint types.

Another nice feature would be a function that can be called from a sql
statement and would throw an exception when executed.

I know these functions can be implemented using UDF, but the performance
and need to deploy it to every database is very inconvenient.

Thank you

Re: proposal - log_full_scan

[Pavel Stehule]

so 17. 4. 2021 v 18:54 odesílatel Julien Rouhaud 
napsal:

> On Sat, Apr 17, 2021 at 05:22:59PM +0200, Pavel Stehule wrote:
> >
> > The fullscan of this table needs about 30ms and has 200K rows. So
> > decreasing log_min_duration to this value is very risky.
> >
> > [...]
> >
> > I use  pg_stat_all_tables.seq_scan and I see seq scans there. But I need
> to
> > know the related queries.
>
> Maybe you could use pg_qualstats ([1]) for that?  It will give you the
> list of
> quals (with the underlying queryid) with a tag to specify if they were
> executed
> as an index scan or a sequential scan.  It wouldn't detect queries doing
> sequential scan that don't have any qual for the underlying relations, but
> those shouldn't be a concern in your use case.
>
> If you setup some sampling, the overhead should be minimal.
>
> [1]: https://github.com/powa-team/pg_qualstats/


It has similar functionality - there is a problem with setting. The my idea
is more simple - just

set

log_fullscall_min_tupples = 10

or

alter table xxx set log_fullscan_min_tupples = 0;

and then the complete query can be found in the log.

I think this can be really practical so it can be core functionality. And
it can log the queries without
quals too. The productions systems can be buggy and it is important to find
bugs

Regards

Pavel

Re: proposal - log_full_scan

[Julien Rouhaud]

On Sat, Apr 17, 2021 at 05:22:59PM +0200, Pavel Stehule wrote:
> 
> The fullscan of this table needs about 30ms and has 200K rows. So
> decreasing log_min_duration to this value is very risky.
> 
> [...]
> 
> I use  pg_stat_all_tables.seq_scan and I see seq scans there. But I need to
> know the related queries.

Maybe you could use pg_qualstats ([1]) for that?  It will give you the list of
quals (with the underlying queryid) with a tag to specify if they were executed
as an index scan or a sequential scan.  It wouldn't detect queries doing
sequential scan that don't have any qual for the underlying relations, but
those shouldn't be a concern in your use case.

If you setup some sampling, the overhead should be minimal.

[1]: https://github.com/powa-team/pg_qualstats/

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/17/21 12:31 PM, Andrew Dunstan wrote:
> On 4/12/21 10:57 AM, Jehan-Guillaume de Rorthais wrote:
>> On Mon, 12 Apr 2021 09:52:24 -0400
>> Andrew Dunstan  wrote:
>>
>>> On 4/12/21 8:59 AM, Jehan-Guillaume de Rorthais wrote:
 Hi,

 On Wed, 7 Apr 2021 20:07:41 +0200
 Jehan-Guillaume de Rorthais  wrote:
 [...]  
>>> Let me know if it worth that I work on an official patch.  
>> Let's give it a try ...
> OK  
 So, as promised, here is my take to port my previous work on PostgreSQL
 source tree.

 Make check pass with no errors. The new class probably deserve some own TAP
 tests.

 Note that I added a PostgresVersion class for easier and cleaner version
 comparisons. This could be an interesting take away no matter what.

 I still have some more ideas to cleanup, revamp and extend the base class,
 but I prefer to go incremental to keep things review-ability.
  
>>> Thanks for this. We have been working somewhat on parallel lines. With
>>> your permission I'm going to take some of what you've done and
>>> incorporate it in the patch I'm working on.
>> The current context makes my weeks difficult to plan and quite chaotic, 
>> that's
>> why it took me some days to produce the patch I promised.
>>
>> I'm fine with working with a common base code, thanks. Feel free to merge 
>> both
>> code, we'll trade patches during review. However, I'm not sure what is the
>> status of your patch, so I can not judge what would be the easier way to
>> incorporate. Mine is tested down to 9.1 (9.0 was meaningless because of lack 
>> of
>> pg_stat_replication) with:
>>
>> * get_new_node
>> * init(allows_streaming => 1)
>> * start
>> * stop
>> * backup
>> * init_from_backup
>> * wait_for_catchup
>> * command_checks_all
>>
>> Note the various changes in my init() and new method allow_streaming(), etc.
>>
>> FYI (to avoid duplicate work), the next step on my todo was to produce some
>> meaningful tap tests to prove the class.
>>
>>> A PostgresVersion class is a good idea - I was already contemplating
>>> something of the kind.
>> Thanks!
>>
>
> OK, here is more WIP on this item. I have drawn substantially on Mark's
> and Jehan-Guillaime's work, but it doesn't really resemble either, and I
> take full responsibility for it.
>
> The guiding principles have been:
>
> . avoid doing version tests, or capability tests which are the moral
> equivalent, and rely instead on pure overloading.
>
> . avoid overriding large pieces of code.
>
>
> The last has involved breaking up some large subroutines (like init)
> into pieces which can more sensibly be overridden. Breaking them up
> isn't a bad thing to do anyway.
>
> There is a new PostgresVersion object, but it's deliberately very
> minimal. Since we're not doing version tests we don't need more complex
> routines.
>
>


I should have also attached my test program - here it is. Also, I have
been testing with the binaries which I've published here:
 along with some saved by my
buildfarm animal.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com



testpgnode.pl
Description: Perl program

Re: multi-install PostgresNode fails with older postgres versions

[Andrew Dunstan]

On 4/12/21 10:57 AM, Jehan-Guillaume de Rorthais wrote:
> On Mon, 12 Apr 2021 09:52:24 -0400
> Andrew Dunstan  wrote:
>
>> On 4/12/21 8:59 AM, Jehan-Guillaume de Rorthais wrote:
>>> Hi,
>>>
>>> On Wed, 7 Apr 2021 20:07:41 +0200
>>> Jehan-Guillaume de Rorthais  wrote:
>>> [...]  
>> Let me know if it worth that I work on an official patch.  
> Let's give it a try ...
 OK  
>>> So, as promised, here is my take to port my previous work on PostgreSQL
>>> source tree.
>>>
>>> Make check pass with no errors. The new class probably deserve some own TAP
>>> tests.
>>>
>>> Note that I added a PostgresVersion class for easier and cleaner version
>>> comparisons. This could be an interesting take away no matter what.
>>>
>>> I still have some more ideas to cleanup, revamp and extend the base class,
>>> but I prefer to go incremental to keep things review-ability.
>>>  
>> Thanks for this. We have been working somewhat on parallel lines. With
>> your permission I'm going to take some of what you've done and
>> incorporate it in the patch I'm working on.
> The current context makes my weeks difficult to plan and quite chaotic, that's
> why it took me some days to produce the patch I promised.
>
> I'm fine with working with a common base code, thanks. Feel free to merge both
> code, we'll trade patches during review. However, I'm not sure what is the
> status of your patch, so I can not judge what would be the easier way to
> incorporate. Mine is tested down to 9.1 (9.0 was meaningless because of lack 
> of
> pg_stat_replication) with:
>
> * get_new_node
> * init(allows_streaming => 1)
> * start
> * stop
> * backup
> * init_from_backup
> * wait_for_catchup
> * command_checks_all
>
> Note the various changes in my init() and new method allow_streaming(), etc.
>
> FYI (to avoid duplicate work), the next step on my todo was to produce some
> meaningful tap tests to prove the class.
>
>> A PostgresVersion class is a good idea - I was already contemplating
>> something of the kind.
> Thanks!
>


OK, here is more WIP on this item. I have drawn substantially on Mark's
and Jehan-Guillaime's work, but it doesn't really resemble either, and I
take full responsibility for it.

The guiding principles have been:

. avoid doing version tests, or capability tests which are the moral
equivalent, and rely instead on pure overloading.

. avoid overriding large pieces of code.


The last has involved breaking up some large subroutines (like init)
into pieces which can more sensibly be overridden. Breaking them up
isn't a bad thing to do anyway.

There is a new PostgresVersion object, but it's deliberately very
minimal. Since we're not doing version tests we don't need more complex
routines.


cheers


andrew




--
Andrew Dunstan
EDB: https://www.enterprisedb.com

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index e209ea7163..1d0866eea7 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -96,6 +96,7 @@ use File::Spec;
 use File::stat qw(stat);
 use File::Temp ();
 use IPC::Run;
+use PostgresVersion;
 use RecursiveCopy;
 use Socket;
 use Test::More;
@@ -127,6 +128,20 @@ INIT
 	$last_port_assigned = int(rand() * 16384) + 49152;
 }
 
+# Current dev version, for which we have no subclass
+# When a new stable branch is made this and the subclass hierarchy below
+# need to be adjusted.
+my $devtip = 14;
+
+INIT
+{
+	# sanity check to make sure there is a subclass for the last stable branch
+	my $last_child = 'PostgresNodeV_' . ($devtip -1);
+	eval "${last_child}->can('get_new_node') || die('not found');";
+	die "No child package $last_child found" if $@;
+}
+
+
 =pod
 
 =head1 METHODS
@@ -347,9 +362,12 @@ about this node.
 sub info
 {
 	my ($self) = @_;
+	my $varr = $self->{_pg_version};
+	my $vstr = join('.', @$varr) if ref $varr;
 	my $_info = '';
 	open my $fh, '>', \$_info or die;
 	print $fh "Name: " . $self->name . "\n";
+	print $fh "Version: " . $vstr . "\n" if $vstr;
 	print $fh "Data directory: " . $self->data_dir . "\n";
 	print $fh "Backup directory: " . $self->backup_dir . "\n";
 	print $fh "Archive directory: " . $self->archive_dir . "\n";
@@ -438,7 +456,7 @@ sub init
 	mkdir $self->backup_dir;
 	mkdir $self->archive_dir;
 
-	TestLib::system_or_bail('initdb', '-D', $pgdata, '-A', 'trust', '-N',
+	TestLib::system_or_bail('initdb', '-D', $pgdata, ($self->_initdb_flags),
 		@{ $params{extra} });
 	TestLib::system_or_bail($ENV{PG_REGRESS}, '--config-auth', $pgdata,
 		@{ $params{auth_extra} });
@@ -446,11 +464,11 @@ sub init
 	open my $conf, '>>', "$pgdata/postgresql.conf";
 	print $conf "\n# Added by PostgresNode.pm\n";
 	print $conf "fsync = off\n";
-	print $conf "restart_after_crash = off\n";
-	print $conf "log_line_prefix = '%m [%p] %q%a '\n";
-	print $conf "log_statement = all\n";
-	print $conf "log_replication_commands = on\n";
-	print $conf "wal_retrieve_retry_interval = '500ms'\n";
+	print $conf $self->_init_restart_after_crash;
+	print $conf 

Re: proposal - log_full_scan

[Pavel Stehule]

so 17. 4. 2021 v 17:09 odesílatel Justin Pryzby 
napsal:

> On Sat, Apr 17, 2021 at 04:36:52PM +0200, Pavel Stehule wrote:
> > today I worked on postgres's server used for critical service. Because
> the
> > application is very specific, we had to do final tuning on production
> > server. I fix lot of queries, but I am not able to detect fast queries
> that
> > does full scan of middle size tables - to 1M rows. Surely I wouldn't log
> > all queries. Now, there are these queries with freq 10 per sec.
> >
> > Can be nice to have a possibility to set a log of  queries that do full
> > scan and read more tuples than is specified limit or that does full scan
> of
> > specified tables.
> >
> > What do you think about the proposed feature?
>
> Are you able to use auto_explain with auto_explain.log_min_duration ?
>

Unfortunately,  I cannot use it. This server executes 5K queries per
seconds, and I am afraid to decrease log_min_duration.

The logs are forwarded to the network and last time, when users played with
it, then they had problems with the network.

I am in a situation where I know there are queries faster than 100ms, I see
so there should be fullscans from pg_stat_user_tables, but I don't see the
queries.

The fullscan of this table needs about 30ms and has 200K rows. So
decreasing log_min_duration to this value is very risky.



> Then you can search for query logs with
> message ~ 'Seq Scan .* \(actual time=[.0-9]* rows=[0-9]{6,} loops=[0-9]*)'
>
> Or can you use pg_stat_all_tables.seq_scan ?
>

I use  pg_stat_all_tables.seq_scan and I see seq scans there. But I need to
know the related queries.


> But it seems to me that filtering on the duration would be both a more
> important criteria and a more general one, than "seq scan with number of
> rows".
>
> | (split_part(message, ' ', 2)::float/1000 AS duration ..) WHERE
> duration>;
>
> --
> Justin
>

Re: proposal - log_full_scan

[Justin Pryzby]

On Sat, Apr 17, 2021 at 04:36:52PM +0200, Pavel Stehule wrote:
> today I worked on postgres's server used for critical service. Because the
> application is very specific, we had to do final tuning on production
> server. I fix lot of queries, but I am not able to detect fast queries that
> does full scan of middle size tables - to 1M rows. Surely I wouldn't log
> all queries. Now, there are these queries with freq 10 per sec.
> 
> Can be nice to have a possibility to set a log of  queries that do full
> scan and read more tuples than is specified limit or that does full scan of
> specified tables.
> 
> What do you think about the proposed feature?

Are you able to use auto_explain with auto_explain.log_min_duration ?

Then you can search for query logs with
message ~ 'Seq Scan .* \(actual time=[.0-9]* rows=[0-9]{6,} loops=[0-9]*)'

Or can you use pg_stat_all_tables.seq_scan ?

But it seems to me that filtering on the duration would be both a more
important criteria and a more general one, than "seq scan with number of rows".

| (split_part(message, ' ', 2)::float/1000 AS duration ..) WHERE duration>;

-- 
Justin

proposal - log_full_scan

[Pavel Stehule]

Hi

today I worked on postgres's server used for critical service. Because the
application is very specific, we had to do final tuning on production
server. I fix lot of queries, but I am not able to detect fast queries that
does full scan of middle size tables - to 1M rows. Surely I wouldn't log
all queries. Now, there are these queries with freq 10 per sec.

Can be nice to have a possibility to set a log of  queries that do full
scan and read more tuples than is specified limit or that does full scan of
specified tables.

What do you think about the proposed feature?

Regards

Pavel

Re: File truncation within PostgresNode::issues_sql_like() wrong on Windows

[Andrew Dunstan]

On 4/17/21 9:04 AM, Michael Paquier wrote:
> On Thu, Apr 15, 2021 at 09:12:52PM -0400, Andrew Dunstan wrote:
>> It's worked on fairywren, I will double check on drongo and if all is
>> well will commit.
> Thanks Andrew.  For the archive's sake, this has been committed as of
> 3c5b068.
>
> While reading the commit, I have noticed that you used SEEK_SET
> instead of 0 as I did in my own patch.  That makes the code easier to
> understand.  Could it be better to apply the same style to all the
> perl scripts doing some seek() calls?  Please see the attached.



Yes please, much better to use a symbolic name rather than a magic
number. I wouldn't bother backpatching it though.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Table refer leak in logical replication

[Amit Kapila]

On Fri, Apr 16, 2021 at 11:55 AM Michael Paquier  wrote:
>
> On Tue, Apr 06, 2021 at 02:25:05PM +0900, Amit Langote wrote:
>
> Attached is v5 that I am finishing with.  Much more could be done but
> I don't want to do something too invasive at this stage of the game.
> There are a couple of extra relations in terms of relations opened for
> a partitioned table within create_estate_for_relation() when
> redirecting to the tuple routing, but my guess is that this would be
> better in the long-term.
>

Hmm, I am not sure if it is a good idea to open indexes needlessly
especially when it is not done in the previous code.

@@ -1766,8 +1771,11 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
  slot_getallattrs(remoteslot);
  }
  MemoryContextSwitchTo(oldctx);
+
+ ExecOpenIndices(partrelinfo_new, false);
  apply_handle_insert_internal(partrelinfo_new, estate,
  remoteslot_part);
+ ExecCloseIndices(partrelinfo_new);
  }

It seems you forgot to call open indexes before apply_handle_delete_internal.

I am not sure if it is a good idea to do the refactoring related to
indexes or other things to fix a minor bug in commit 1375422c. It
might be better to add a simple fix like what Hou-San has originally
proposed [1] because even using ExecInitResultRelation might not be
the best thing as it is again trying to open a range table which we
have already opened in logicalrep_rel_open. OTOH, using
ExecInitResultRelation might encapsulate the assignment we are doing
outside. In general, it seems doing bigger refactoring or changes
might lead to some bugs or unintended consequences, so if possible, we
can try such refactoring as a separate patch. One argument against the
proposed refactoring could be that with the previous code, we were
trying to open the indexes just before it is required but with the new
patch in some cases, they will be opened during the initial phase and
for other cases, they are opened just before they are required. It
might not necessarily be a bad idea to rearrange code like that but
maybe there is a better way to do that.

[1] - 
https://www.postgresql.org/message-id/OS0PR01MB571686F75FBDC219FF3DFF0D94769%40OS0PR01MB5716.jpnprd01.prod.outlook.com

-- 
With Regards,
Amit Kapila.

Re: File truncation within PostgresNode::issues_sql_like() wrong on Windows

[Michael Paquier]

On Thu, Apr 15, 2021 at 09:12:52PM -0400, Andrew Dunstan wrote:
> It's worked on fairywren, I will double check on drongo and if all is
> well will commit.

Thanks Andrew.  For the archive's sake, this has been committed as of
3c5b068.

While reading the commit, I have noticed that you used SEEK_SET
instead of 0 as I did in my own patch.  That makes the code easier to
understand.  Could it be better to apply the same style to all the
perl scripts doing some seek() calls?  Please see the attached.
--
Michael
diff --git a/src/bin/pg_amcheck/t/003_check.pl b/src/bin/pg_amcheck/t/003_check.pl
index 66dd14e498..2da9631da4 100644
--- a/src/bin/pg_amcheck/t/003_check.pl
+++ b/src/bin/pg_amcheck/t/003_check.pl
@@ -3,6 +3,8 @@ use warnings;
 
 use PostgresNode;
 use TestLib;
+
+use Fcntl qw(:seek);
 use Test::More tests => 63;
 
 my ($node, $port, %corrupt_page, %remove_relation);
@@ -84,7 +86,7 @@ sub corrupt_first_page
 	# Corrupt some line pointers.  The values are chosen to hit the
 	# various line-pointer-corruption checks in verify_heapam.c
 	# on both little-endian and big-endian architectures.
-	seek($fh, 32, 0)
+	seek($fh, 32, SEEK_SET)
 		or BAIL_OUT("seek failed: $!");
 	syswrite(
 		$fh,
diff --git a/src/bin/pg_amcheck/t/004_verify_heapam.pl b/src/bin/pg_amcheck/t/004_verify_heapam.pl
index 3c1277adf3..b842f7bc6d 100644
--- a/src/bin/pg_amcheck/t/004_verify_heapam.pl
+++ b/src/bin/pg_amcheck/t/004_verify_heapam.pl
@@ -4,6 +4,7 @@ use warnings;
 use PostgresNode;
 use TestLib;
 
+use Fcntl qw(:seek);
 use Test::More;
 
 # This regression test demonstrates that the pg_amcheck binary correctly
@@ -95,7 +96,7 @@ sub read_tuple
 {
 	my ($fh, $offset) = @_;
 	my ($buffer, %tup);
-	seek($fh, $offset, 0)
+	seek($fh, $offset, SEEK_SET)
 		or BAIL_OUT("seek failed: $!");
 	defined(sysread($fh, $buffer, HEAPTUPLE_PACK_LENGTH))
 		or BAIL_OUT("sysread failed: $!");
@@ -172,7 +173,7 @@ sub write_tuple
 	$tup->{c_va_extinfo},
 	$tup->{c_va_valueid},
 	$tup->{c_va_toastrelid});
-	seek($fh, $offset, 0)
+	seek($fh, $offset, SEEK_SET)
 		or BAIL_OUT("seek failed: $!");
 	defined(syswrite($fh, $buffer, HEAPTUPLE_PACK_LENGTH))
 		or BAIL_OUT("syswrite failed: $!");
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 089c9cb851..a9dfe88aaa 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -4,6 +4,7 @@ use Cwd;
 use Config;
 use File::Basename qw(basename dirname);
 use File::Path qw(rmtree);
+use Fcntl qw(:seek);
 use PostgresNode;
 use TestLib;
 use Test::More tests => 110;
@@ -555,7 +556,7 @@ my $block_size  = $node->safe_psql('postgres', 'SHOW block_size;');
 # induce corruption
 system_or_bail 'pg_ctl', '-D', $pgdata, 'stop';
 open $file, '+<', "$pgdata/$file_corrupt1";
-seek($file, $pageheader_size, 0);
+seek($file, $pageheader_size, SEEK_SET);
 syswrite($file, "\0\0\0\0\0\0\0\0\0");
 close $file;
 system_or_bail 'pg_ctl', '-D', $pgdata, 'start';
@@ -574,7 +575,7 @@ open $file, '+<', "$pgdata/$file_corrupt1";
 for my $i (1 .. 5)
 {
 	my $offset = $pageheader_size + $i * $block_size;
-	seek($file, $offset, 0);
+	seek($file, $offset, SEEK_SET);
 	syswrite($file, "\0\0\0\0\0\0\0\0\0");
 }
 close $file;
@@ -591,7 +592,7 @@ rmtree("$tempdir/backup_corrupt2");
 # induce corruption in a second file
 system_or_bail 'pg_ctl', '-D', $pgdata, 'stop';
 open $file, '+<', "$pgdata/$file_corrupt2";
-seek($file, $pageheader_size, 0);
+seek($file, $pageheader_size, SEEK_SET);
 syswrite($file, "\0\0\0\0\0\0\0\0\0");
 close $file;
 system_or_bail 'pg_ctl', '-D', $pgdata, 'start';
diff --git a/src/bin/pg_checksums/t/002_actions.pl b/src/bin/pg_checksums/t/002_actions.pl
index 8a81f36a06..d52bbac5fa 100644
--- a/src/bin/pg_checksums/t/002_actions.pl
+++ b/src/bin/pg_checksums/t/002_actions.pl
@@ -5,6 +5,8 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
+
+use Fcntl qw(:seek);
 use Test::More tests => 63;
 
 
@@ -50,7 +52,7 @@ sub check_relation_corruption
 
 	# Time to create some corruption
 	open my $file, '+<', "$pgdata/$file_corrupted";
-	seek($file, $pageheader_size, 0);
+	seek($file, $pageheader_size, SEEK_SET);
 	syswrite($file, "\0\0\0\0\0\0\0\0\0");
 	close $file;
 
diff --git a/contrib/amcheck/t/001_verify_heapam.pl b/contrib/amcheck/t/001_verify_heapam.pl
index 6050feb712..bf47c2ed37 100644
--- a/contrib/amcheck/t/001_verify_heapam.pl
+++ b/contrib/amcheck/t/001_verify_heapam.pl
@@ -4,6 +4,7 @@ use warnings;
 use PostgresNode;
 use TestLib;
 
+use Fcntl qw(:seek);
 use Test::More tests => 80;
 
 my ($node, $result);
@@ -124,7 +125,7 @@ sub corrupt_first_page
 	# Corrupt some line pointers.  The values are chosen to hit the
 	# various line-pointer-corruption checks in verify_heapam.c
 	# on both little-endian and big-endian architectures.
-	seek($fh, 32, 0)
+	seek($fh, 32, SEEK_SET)
 	  or BAIL_OUT("seek failed: $!");
 	syswrite(
 		$fh,


signature.asc
Description: PGP 

Re: Error when defining a set returning function

[Esteban Zimanyi]

> If you build with pgxs it should supply the appropriate compiler flags.
> Alternatively, get the right settings from pg_config. In general rolling
> your own is a bad idea.
>

I didn't know about pgxs. Many thanks Andrew for pointing this out.

Re: Bogus collation version recording in recordMultipleDependencies

[Julien Rouhaud]

On Fri, Apr 16, 2021 at 10:24:21PM -0400, Tom Lane wrote:
> Thomas Munro  writes:
> > On Sat, Apr 17, 2021 at 10:47 AM Tom Lane  wrote:
> >> Although there are only a few buildfarm members complaining, I don't
> >> really want to leave them red all weekend.  I could either commit the
> >> patch I just presented, or revert ef387bed8 ... got a preference?
> 
> > +1 for committing the new patch for now.  I will look into to the
> > record problem.  More in a couple of days.
> 
> OK, done.

Thanks for the fixes!  I'll also look at the problem.

Re: Bogus collation version recording in recordMultipleDependencies

[Julien Rouhaud]

On Fri, Apr 16, 2021 at 01:07:52PM -0400, Tom Lane wrote:
> I wrote:
> > ... or maybe not just yet.  Andres' buildfarm critters seem to have
> > a different opinion than my machine about what the output of
> > collate.icu.utf8 ought to be.  I wonder what the prevailing LANG
> > setting is for them, and which ICU version they're using.
> 
> Oh, I bet it's "C.utf8", because I can reproduce the failure with that.
> This crystallizes a nagging feeling I'd had that you were misdescribing
> the collate.icu.utf8 test as not being run under --no-locale.  Actually,
> it's only skipped if the encoding isn't UTF8, not the same thing.
> I think we need to remove the default-collation cases from that test too.

IIUC pg_regress --no-locale will call initdb --no-locale which force the locale
to C, and in that case pg_get_encoding_from_locale() does force SQL_ASCII as
encoding.  But yes I clearly didn't think at all that you could set the various
env variables to C.utf8 which can then run the collate.icu.utf8 or linux.utf8
:(

Re: Truncate in synchronous logical replication failed

[Japin Li]

On Sat, 17 Apr 2021 at 12:03, osumi.takami...@fujitsu.com 
 wrote:
> On Saturday, April 17, 2021 12:53 AM Japin Li 
>> On Fri, 16 Apr 2021 at 17:19, osumi.takami...@fujitsu.com
>>  wrote:
>> > On Friday, April 16, 2021 5:50 PM Amit Kapila 
>> wrote:
>> >> On Fri, Apr 16, 2021 at 12:56 PM osumi.takami...@fujitsu.com
>> >>  wrote:
>> >> >
>> >> > > Thanks for your reminder.  It might be a way to solve this problem.
>> >> > Yeah. I've made the 1st patch for this issue.
>> >> >
>> >> > In my env, with the patch
>> >> > the TRUNCATE in synchronous logical replication doesn't hang.
>> >> >
>> >>
>> >> Few initial comments:
>> >> =
>> >> 1.
>> >> + relreplindex = relation->rd_replidindex;
>> >> +
>> >> + /*
>> >> + * build attributes to idindexattrs.
>> >> + */
>> >> + idindexattrs = NULL;
>> >> + foreach(l, indexoidlist)
>> >> + {
>> >> + Oid indexOid = lfirst_oid(l);
>> >> + Relation indexDesc;
>> >> + int i;
>> >> + bool isIDKey; /* replica identity index */
>> >> +
>> >> + indexDesc = RelationIdGetRelation(indexOid);
>> >>
>> >> When you have oid of replica identity index (relreplindex) then what
>> >> is the need to traverse all the indexes?
>> > Ok. No need to traverse all the indexes. Will fix this part.
>> >
>> >> 2.
>> >> It is better to name the function as RelationGet...
>> > You are right. I'll modify this in my next version.
>> >
>>
>> I took the liberty to address review comments and provide a v2 patch on top
>> of your's v1 patch, also merge the test case.
>>
>> Sorry for attaching.
> No problem. Thank you for updating the patch.
> I've conducted some cosmetic changes. Could you please check this ?
> That's already applied by pgindent.
>
> I executed RT for this and made no failure.
> Just in case, I executed 010_truncate.pl test 100 times in a tight loop,
> which also didn't fail.
>

LGTM, I made an entry in the commitfest[1], so that the patches will get a
chance to run on all the platforms.

[1] - https://commitfest.postgresql.org/33/3081/

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: Forget close an open relation in ReorderBufferProcessTXN()

[Amit Kapila]

On Sat, Apr 17, 2021 at 12:05 PM Japin Li  wrote:
>
> On Sat, 17 Apr 2021 at 14:09, Amit Kapila  wrote:
> > On Thu, Apr 15, 2021 at 4:53 PM Amit Kapila  wrote:
> >>
> >> On Thu, Apr 15, 2021 at 4:00 PM Japin Li  wrote:
> >> >
> >> > The RelationIdGetRelation() comment says:
> >> >
> >> > > Caller should eventually decrement count. (Usually,
> >> > > that happens by calling RelationClose().)
> >> >
> >> > However, it doesn't do it in ReorderBufferProcessTXN().
> >> > I think we should close it, here is a patch that fixes it. Thoughts?
> >> >
> >>
> >> +1. Your fix looks correct to me but can we test it in some way?
> >>
> >
> > I have tried to find a test but not able to find one. I have tried
> > with a foreign table but we don't log truncate for it, see
> > ExecuteTruncate. It has a check that it will log for relids where
> > RelationIsLogicallyLogged. If that is the case, it is not clear to me
> > how we can ever hit this condition? Have you tried to find the test?
>
> I also don't find a test for this.  It is introduced in 5dfd1e5a6696,
> wrote by Simon Riggs, Marco Nenciarini and Peter Eisentraut.  Maybe they
> can explain when we can enter this condition?
>

My guess is that this has been copied from the code a few lines above
to handle insert/update/delete where it is required to handle some DDL
ops like Alter Table but I think we don't need it here (for Truncate
op). If that understanding turns out to be true then we should either
have an Assert for this or an elog message.

-- 
With Regards,
Amit Kapila.

Re: Forget close an open relation in ReorderBufferProcessTXN()

[Japin Li]

On Sat, 17 Apr 2021 at 14:09, Amit Kapila  wrote:
> On Thu, Apr 15, 2021 at 4:53 PM Amit Kapila  wrote:
>>
>> On Thu, Apr 15, 2021 at 4:00 PM Japin Li  wrote:
>> >
>> > The RelationIdGetRelation() comment says:
>> >
>> > > Caller should eventually decrement count. (Usually,
>> > > that happens by calling RelationClose().)
>> >
>> > However, it doesn't do it in ReorderBufferProcessTXN().
>> > I think we should close it, here is a patch that fixes it. Thoughts?
>> >
>>
>> +1. Your fix looks correct to me but can we test it in some way?
>>
>
> I have tried to find a test but not able to find one. I have tried
> with a foreign table but we don't log truncate for it, see
> ExecuteTruncate. It has a check that it will log for relids where
> RelationIsLogicallyLogged. If that is the case, it is not clear to me
> how we can ever hit this condition? Have you tried to find the test?

I also don't find a test for this.  It is introduced in 5dfd1e5a6696,
wrote by Simon Riggs, Marco Nenciarini and Peter Eisentraut.  Maybe they
can explain when we can enter this condition?

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: Forget close an open relation in ReorderBufferProcessTXN()

[Amit Kapila]

On Fri, Apr 16, 2021 at 11:24 PM Andres Freund  wrote:
>
>
> > I think it is also important to *not* acquire any lock on relation
> > otherwise it can lead to some sort of deadlock or infinite wait in the
> > decoding process. Consider a case for operations like Truncate (or if
> > the user has acquired an exclusive lock on the relation in some other
> > way say via Lock command) which acquires an exclusive lock on
> > relation, it won't get replicated in synchronous mode (when
> > synchronous_standby_name is configured). The truncate operation will
> > wait for the transaction to be replicated to the subscriber and the
> > decoding process will wait for the Truncate operation to finish.
>
> However, this cannot be really relied upon for catalog tables. An output
> function might acquire locks or such. But for those we do not need to
> decode contents...
>

I see that if we define a user_catalog_table (create table t1_cat(c1
int) WITH(user_catalog_table = true);), we are able to decode
operations like (insert, truncate) on such a table. What do you mean
by "But for those we do not need to decode contents"?

-- 
With Regards,
Amit Kapila.

Re: Forget close an open relation in ReorderBufferProcessTXN()

[Amit Kapila]

On Thu, Apr 15, 2021 at 4:53 PM Amit Kapila  wrote:
>
> On Thu, Apr 15, 2021 at 4:00 PM Japin Li  wrote:
> >
> > The RelationIdGetRelation() comment says:
> >
> > > Caller should eventually decrement count. (Usually,
> > > that happens by calling RelationClose().)
> >
> > However, it doesn't do it in ReorderBufferProcessTXN().
> > I think we should close it, here is a patch that fixes it. Thoughts?
> >
>
> +1. Your fix looks correct to me but can we test it in some way?
>

I have tried to find a test but not able to find one. I have tried
with a foreign table but we don't log truncate for it, see
ExecuteTruncate. It has a check that it will log for relids where
RelationIsLogicallyLogged. If that is the case, it is not clear to me
how we can ever hit this condition? Have you tried to find the test?

-- 
With Regards,
Amit Kapila.