Re: Speed up transaction completion faster after many relations are accessed in a transaction
On Mon, 21 Jun 2021 at 01:56, David Rowley wrote: > # A new hashtable implementation > Also, it would be good to hear what people think about solving the > problem this way. Because over on [1] I'm also trying to improve the performance of smgropen(), I posted the patch for the new hash table over there too. Between that thread and discussions with Thomas and Andres off-list, I get the idea that pretty much nobody likes the idea of having another hash table implementation. Thomas wants to solve it another way and Andres has concerns that working with bitmaps is too slow. Andres suggested freelists would be faster, but I'm not really a fan of the idea as, unless I have a freelist per array segment then I won't be able to quickly identify the earliest segment slot to re-fill unused slots with. That would mean memory would get more fragmented over time instead of the fragmentation being slowly fixed as new items are added after deletes. So I've not really tried implementing that to see how it performs. Both Andres and Thomas expressed a dislike to the name "generichash" too. Anyway, since I did make a few small changes to the hash table implementation before doing all that off-list talking, I thought I should at least post where I got to here so that anything else that comes up can get compared to where I got to, instead of where I was with this. I did end up renaming the hash table to "densehash" rather than generichash. Naming is hard, but I went with dense as memory density was on my mind when I wrote it. Having a compact 8-byte bucket width and packing the data into arrays in a dense way. The word dense came up a few times, so went with that. I also adjusted the hash seq scan code so that it performs better when faced a non-sparsely populated table. Previously my benchmark for that case didn't do well [2]. I've attached the benchmark results from running the benchmark that's included in hashbench.tar.bz2. I ran this 10 times using the included test.sh with ./test.sh 10. I included the results I got on my AMD machine in the attached bz2 file in results.csv. You can see from the attached dense_vs_generic_vs_simple.png that dense hash is quite comparable to simplehash for inserts/deletes and lookups. It's not quite as fast as simplehash at iterations when the table is not bloated, but blows simplehash out the water when the hashtables have become bloated due to having once contained a large number of records but no longer do. Anyway, unless there is some interest in me taking this idea further then, due to the general feedback received on [1], I'm not planning on pushing this any further. I'll leave the commitfest entry as is for now to give others a chance to see this. David [1] https://postgr.es/m/CAApHDvowgRaQupC%3DL37iZPUzx1z7-N8deD7TxQSm8LR%2Bf4L3-A%40mail.gmail.com [2] https://www.postgresql.org/message-id/CAApHDvpuzJTQNKQ_bnAccvi-68xuh%2Bv87B4P6ycU-UiN0dqyTg%40mail.gmail.com hashbench.tar.bz2 Description: Binary data densehash_for_lockreleaseall.patch Description: Binary data
Re: Support kerberos authentication for postgres_fdw
Sorry I have sent a duplicate email. I will first continue discussion in the other thread and then submit it after we have a conclusion. Thanks. Peifeng
Re: Support kerberos authentication for postgres_fdw
>As you note, this'd have to be restricted to superusers, which makes it >seem like a pretty bad idea. We really don't want to be in a situation >of pushing people to run day-to-day stuff as superuser. Yeah, having >access to kerberos auth sounds good on the surface, but it seems like >it would be a net loss in security because of that. I can imagine the use case would be a superuser creates the user mapping and foreign table, then grants access of foreign table to a normal user. This way the normal user can execute queries on the foreign table but can't access sensitive information in user mapping. The main purpose of this patch is to provide a simple way to do kerberos authentication with the least modification possible. >ISTM the right way to do this would be using Kerberos delegation. That >is, the system would be set up so that the postgres service principal >is trusted for kerberos delegation and it would then pass through the >actual Kerberos authentication from the client. I agree this sounds like the ideal solution. If I understand it correctly, this approach requires both postgres servers to use same kerberos settings(kdc, realm, etc), and the FDW server can just "forward" necessary information to authenticate on behalf of the same user. I will spend some time to investigate it and reach out later. Best regards, Peifeng
Re: Protect against possible memory corruption (src/backend/access/nbtree/nbtxlog.c)
On 12/07/2021 02:34, Ranier Vilela wrote:
If it is not possible, know the upper limits, before the loop.
It is necessary to do this inside the loop.
@@ -49,10 +47,14 @@ _bt_restore_page(Page page, char *from, int len)
* To get the items back in the original order, we add them to the page
in
* reverse. To figure out where one tuple ends and another begins, we
* have to scan them in forward order first.
+* Check the array upper limit to not overtake him.
*/
i = 0;
- while (from < end)
+ while (from < end && i <= MaxIndexTuplesPerPage)
{
+ IndexTupleData itupdata;
+ Sizeitemsz;
+
/*
* As we step through the items, 'from' won't always be properly
* aligned, so we need to use memcpy(). Further, we use Item
(which
If we bother checking it, we should throw an error if the check fails,
not just silently soldier on. Also, shouldn't it be '<', not '<='? In
general though, we don't do much checking on WAL records, we assume that
the contents are sane. It would be nice to add more checks and make WAL
redo routines more robust to corrupt records, but this seems like an odd
place to start.
I committed the removal of bogus assignment to 'from'. Thanks!
- Heikki
Re: Can a child process detect postmaster death when in pg_usleep?
On Tue, Jul 06, 2021 at 05:07:04PM +0530, Bharath Rupireddy wrote: > Thanks. Anyways, I removed WL_LATCH_SET for PreAuthDelay as > well. PSA v4 patch. For the moment, please note that I have marked the patch as committed in the CF app. It may be better to start a new thread with the remaining bits for a separate evaluation. -- Michael signature.asc Description: PGP signature
Re: Column Filtering in Logical Replication
Hi Peter, Hi, I was wondering if/when a subset of cols is specified then does > that mean it will be possible for the table to be replicated to a > *smaller* table at the subscriber side? > e.g Can a table with 7 cols replicated to a table with 2 cols? > > table tab1(a,b,c,d,e,f,g) --> CREATE PUBLICATION pub1 FOR TABLE > tab1(a,b) --> table tab1(a,b) > > ~~ > I thought maybe that should be possible, but the expected behaviour > for that scenario was not very clear to me from the thread/patch > comments. And the new TAP test uses the tab1 table created exactly the > same for pub/sub, so I couldn't tell from the test code either. > Currently, this capability is not included in the patch. If the table on the subscriber server has lesser attributes than that on the publisher server, it throws an error at the time of CREATE SUBSCRIPTION. About having such a functionality, I don't immediately see any issue with it as long as we make sure replica identity columns are always present on both instances. However, need to carefully consider situations in which a server subscribes to multiple publications, each publishing a different subset of columns of a table. Thank you, Rahila Syed
Re: Remove useless int64 range checks on BIGINT sequence MINVALUE/MAXVALUE values
On Mon, 12 Jul 2021 at 16:48, Greg Nancarrow wrote: > > On Mon, Jul 12, 2021 at 2:26 PM David Rowley wrote: > > > > > It seems strange to add a comment to explain why it's there. If we're > > > going to the trouble of doing that, then we should just remove it and > > > add a very small comment to mention why INT8 sequences don't need to > > > be checked. > > > > Any thoughts on this, Greg? > > > > The patch LGTM (it's the same as my original patch but with short comments). Yeah, it's your patch with the comment reduced down to 2 lines. This was to try and address Peter's concern that the comment is too large. This seemed to put him off the patch. I also disagreed that it made sense to remove 2 fairly harmless lines of code to replace them with 12 lines of comments. What I was trying to get to here was something that was more reasonable that might make sense to commit. I'm just not certain where Peter stands on this now that the latest patch is a net zero when it comes to adding lines. Peter? David
Re: Added schema level support for publication.
On Fri, Jul 9, 2021 at 12:12 PM Greg Nancarrow wrote: > > On Fri, Jul 9, 2021 at 1:28 PM houzj.f...@fujitsu.com > wrote: > > > > Currently, postgres caches publication actions info in the > > RelationData::rd_pubactions, but after applying the patch, it seems > > rd_pubactions is not initialized when using schema level publication. > > > > It cound result in some unexpected behaviour when checking if command can be > > executed with current replica identity. > > > > While testing this patch, I'm finding that for a FOR SCHEMA > publication, UPDATEs and DELETEs on tables belonging to that schema > are not getting replicated (but INSERTs and TRUNCATEs are). > Could this be related to the issues that Hou-san has identified? Thanks for reporting this issue. I felt this issue is the same as the issue which Hou San had reported. This issue is fixed in the v10 patch attached at [1]. [1] - https://www.postgresql.org/message-id/CALDaNm2%2BtR%2B8R-sD1CSyMbZcZbkintZE-avefjsp7LCkm6HMmw%40mail.gmail.com Regards, Vignesh
Re: row filtering for logical replication
On 7/12/21 6:46 AM, Amit Kapila wrote:
On Mon, Jul 12, 2021 at 7:19 AM Alvaro Herrera wrote:
Hi
Andres complained about the safety of doing general expression
evaluation in pgoutput; that was first in
https://postgr.es/m/20210128022032.eq2qqc6zxkqn5...@alap3.anarazel.de
where he described a possible approach to handle it by restricting
expressions to have limited shape; and later in
http://postgr.es/m/20210331191710.kqbiwe73lur7j...@alap3.anarazel.de
I was just scanning the patch trying to see if some sort of protection
had been added for this, but I couldn't find anything. (Some functions
are under-commented, though). So, is it there already, and if so what
is it?
I think the patch is trying to prohibit arbitrary expressions in the
WHERE clause via
transformWhereClause(..EXPR_KIND_PUBLICATION_WHERE..). You can notice
that at various places the expressions are prohibited via
EXPR_KIND_PUBLICATION_WHERE. I am not sure that the checks are correct
and sufficient but I think there is some attempt to do it. For
example, the below sort of ad-hoc check for func_call doesn't seem to
be good idea.
@@ -119,6 +119,13 @@ transformExprRecurse(ParseState *pstate, Node *expr)
/* Guard against stack overflow due to overly complex expressions */
check_stack_depth();
+ /* Functions are not allowed in publication WHERE clauses */
+ if (pstate->p_expr_kind == EXPR_KIND_PUBLICATION_WHERE &&
nodeTag(expr) == T_FuncCall)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("functions are not allowed in publication WHERE expressions"),
+ parser_errposition(pstate, exprLocation(expr;
Yes, I mentioned this bit of code in my review, although I was mostly
wondering if this is the wrong place to make this check.
Now, the other idea I had in mind was to traverse the WHERE clause
expression in publication_add_relation and identify if it contains
anything other than the ANDed list of 'foo.bar op constant'
expressions. OTOH, for index where clause expressions or policy check
expressions, we use a technique similar to what we have in the patch
to prohibit certain kinds of expressions.
Do you have any preference on how this should be addressed?
I don't think this is sufficient, because who knows where "op" comes
from? It might be from an extension, in which case the problem pointed
out by Petr Jelinek [1] would apply. OTOH I suppose we could allow
expressions like (Var op Var), i.e. "a < b" or something like that. And
then why not allow (a+b < c-10) and similar "more complex" expressions,
as long as all the operators are built-in?
In terms of implementation, I think there are two basic options - either
we can define a new "expression" type in gram.y, which would be a subset
of a_expr etc. Or we can do it as some sort of expression walker, kinda
like what the transform* functions do now.
regards
[1]
https://www.postgresql.org/message-id/92e5587d-28b8-5849-2374-5ca3863256f1%402ndquadrant.com
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: Teach pg_receivewal to use lz4 compression
‐‐‐ Original Message ‐‐‐ On Monday, July 12th, 2021 at 07:56, Michael Paquier wrote: > On Mon, Jul 12, 2021 at 11:10:24AM +0530, Dilip Kumar wrote: > > > On Thu, Jul 8, 2021 at 7:48 PM gkokola...@pm.me wrote: > > > > > We can, though I am not in favour of doing so. There is seemingly > > > > > > little benefit for added complexity. > > > > I am really not sure what complexity you are talking about, do you > > > > mean since with pglz we were already providing the compression level > > > > so let it be as it is but with the new compression method you don't > > > > see much benefit of providing compression level or speed? > > You mean s/pglz/zlib/ here perhaps? I am not sure what Georgios has > > in mind, but my opinion stands on the latter: there is little benefit > > in making lz4 faster than the default and reduce compression, as the > > default is already a rather low CPU user. Thank you all for your comments. I am sitting on the same side as Micheal on this one. The complexity is not huge, yet there will need to be code to pass this option to the lz4 api and various test cases to verify for correctness and integrity. The burden of maintenance of this code vs the benefit of the option, tilt the scale towards not including the option. Of course, I will happily provide whatever the community finds beneficial. Cheers, //Georgios > - > > Michael
Re: Added schema level support for publication.
On Thu, Jul 8, 2021 at 9:16 AM houzj.f...@fujitsu.com
wrote:
>
> On Wednesday, June 30, 2021 7:43 PM vignesh C wrote:
> > Thanks for reporting this issue, the attached v9 patch fixes this issue.
> > This also fixes the other issue you reported at [1].
>
> Hi,
>
> I had a look at the patch, please consider following comments.
>
> (1)
> - if (pub->alltables)
> + if (pub->pubtype == PUBTYPE_ALLTABLES)
> {
> publish = true;
> if (pub->pubviaroot && am_partition)
> publish_as_relid =
> llast_oid(get_partition_ancestors(relid));
> }
>
> + if (pub->pubtype == PUBTYPE_SCHEMA)
> + {
> + Oid schemaId =
> get_rel_namespace(relid);
> + List *pubschemas =
> GetPublicationSchemas(pub->oid);
> +
> + if (list_member_oid(pubschemas, schemaId))
> + {
>
> It might be better use "else if" for the second check here.
> Like: else if (pub->pubtype == PUBTYPE_SCHEMA)
>
> Besides, we already have the {schemaoid, pubid} set here, it might be
> better to scan the cache PUBLICATIONSCHEMAMAP instead of invoking
> GetPublicationSchemas() which will scan the whole table.
Modified.
> (2)
> + /* Identify which schemas should be dropped. */
> + foreach(oldlc, oldschemaids)
> + {
> + Oid oldschemaid =
> lfirst_oid(oldlc);
> + ListCell *newlc;
> + boolfound = false;
> +
> + foreach(newlc, schemaoidlist)
> + {
> + Oid newschemaid =
> lfirst_oid(newlc);
> +
> + if (newschemaid == oldschemaid)
> + {
> + found = true;
> + break;
> + }
> + }
>
> It seems we can use " if (list_member_oid(schemaoidlist, oldschemaid)) "
> to replace the second foreach loop.
>
Modified.
> (3)
> there are some testcases change in 0001 patch, it might be better move them
> to 0002 patch.
These changes are required to modify the existing tests. I kept it in
0001 so that 0001 patch can be committed independently. I think we can
keep the change as it is, I did not make any changes for this.
> (4)
> + case OBJECT_PUBLICATION_SCHEMA:
> + objnode = (Node *) list_make2(linitial(name),
> linitial(args));
> + break;
> case OBJECT_USER_MAPPING:
> objnode = (Node *) list_make2(linitial(name),
> linitial(args));
> break;
>
> Does it looks better to merge these two switch cases ?
> Like:
> case OBJECT_PUBLICATION_SCHEMA:
> case OBJECT_USER_MAPPING:
> objnode = (Node *) list_make2(linitial(name), linitial(args));
> break;
Modified.
Thanks for the comments, these comments are fixed as part of the v10
patch attached at [1].
[1] -
https://www.postgresql.org/message-id/CALDaNm2%2BtR%2B8R-sD1CSyMbZcZbkintZE-avefjsp7LCkm6HMmw%40mail.gmail.com
Regards,
Vignesh
Re: Support kerberos authentication for postgres_fdw
On Mon, Jul 12, 2021 at 5:43 AM Peifeng Qiu wrote: > > >As you note, this'd have to be restricted to superusers, which makes it > >seem like a pretty bad idea. We really don't want to be in a situation > >of pushing people to run day-to-day stuff as superuser. Yeah, having > >access to kerberos auth sounds good on the surface, but it seems like > >it would be a net loss in security because of that. > > I can imagine the use case would be a superuser creates the user > mapping and foreign table, then grants access of foreign table to > a normal user. This way the normal user can execute queries on the > foreign table but can't access sensitive information in user mapping. > > The main purpose of this patch is to provide a simple way to do > kerberos authentication with the least modification possible. But in this case, what dose Kerberos give over just using a password based solution? It adds complexity, but what's teh actual gain? > >ISTM the right way to do this would be using Kerberos delegation. That > >is, the system would be set up so that the postgres service principal > >is trusted for kerberos delegation and it would then pass through the > >actual Kerberos authentication from the client. > > I agree this sounds like the ideal solution. If I understand it correctly, > this approach requires both postgres servers to use same kerberos > settings(kdc, realm, etc), and the FDW server can just "forward" > necessary information to authenticate on behalf of the same user. > I will spend some time to investigate it and reach out later. I don't actually know if they have to be in the same realm, I *think* kerberos delegations work across trusted realms, but I'm not sure about that. -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Re: row filtering for logical replication
On Mon, Jul 12, 2021 at 1:09 AM Euler Taveira wrote: > > I did another measure using as baseline the previous patch (v16). > > without cache (v16) > --- > > mean: 1.46 us > stddev: 2.13 us > median: 1.39 us > min-max:[0.69 .. 1456.69] us > percentile(99): 3.15 us > mode: 0.91 us > > with cache (v18) > --- > > mean: 0.63 us > stddev: 1.07 us > median: 0.55 us > min-max:[0.29 .. 844.87] us > percentile(99): 1.38 us > mode: 0.41 us > > It represents -57%. It is a really good optimization for just a few extra > lines > of code. > Good improvement but I think it is better to measure the performance by using synchronous_replication by setting the subscriber as standby_synchronous_names, which will provide the overall saving of time. We can probably see when the timings when no rows are filtered, when 10% rows are filtered when 30% are filtered and so on. I think the way caching has been done in the patch is a bit inefficient. Basically, it always invalidates and rebuilds the expressions even though some unrelated operation has happened on publication. For example, say publication has initially table t1 with rowfilter r1 for which we have cached the state. Now you altered publication and added table t2, it will invalidate the entire state of t1 as well. I think we can avoid that if we invalidate the rowfilter related state only on relcache invalidation i.e in rel_sync_cache_relation_cb and save it the very first time we prepare the expression. In that case, we don't need to do it in advance when preparing relsyncentry, this will have the additional advantage that we won't spend cycles on preparing state unless it is required (for truncate we won't require row_filtering, so it won't be prepared). Few other things, I have noticed: 1. I am seeing tupledesc leak by following below steps: ERROR: tupdesc reference 008D7D18 is not owned by resource owner TopTransaction CONTEXT: slot "tap_sub", output plugin "pgoutput", in the change callback, associated LSN 0/170BD50 Publisher CREATE TABLE tab_rowfilter_1 (a int primary key, b text); CREATE PUBLICATION tap_pub_1 FOR TABLE tab_rowfilter_1 WHERE (a > 1000 AND b <> 'filtered'); Subscriber CREATE TABLE tab_rowfilter_1 (a int primary key, b text); CREATE SUBSCRIPTION tap_sub CONNECTION 'host=localhost port=5432 dbname=postgres' PUBLICATION tap_pub_1; Publisher INSERT INTO tab_rowfilter_1 (a, b) VALUES (1980, 'not filtered'); Alter table tab_rowfilter_1 drop column b cascade; INSERT INTO tab_rowfilter_1 (a) VALUES (1982); 2. postgres=# Alter table tab_rowfilter_1 alter column b set data type varchar; ERROR: unexpected object depending on column: publication of table tab_rowfilter_1 in publication tap_pub_1 I think for this you need to change ATExecAlterColumnType to handle the publication case. -- With Regards, Amit Kapila.
Re: Added schema level support for publication.
On Fri, Jul 2, 2021 at 10:18 AM tanghy.f...@fujitsu.com wrote: > > On Wednesday, June 30, 2021 7:43 PM vignesh C wrote: > > > > Thanks for reporting this issue, the attached v9 patch fixes this issue. > > This also fixes the other issue you reported at [1]. > > A comment on v9: > > src/bin/psql/describe.c > > + if (pset.sversion >= 15000) > > I think it should be 15. Thanks for reporting this issue, this issue is fixed in the v10 patch attached at [1]. [1] - https://www.postgresql.org/message-id/CALDaNm2%2BtR%2B8R-sD1CSyMbZcZbkintZE-avefjsp7LCkm6HMmw%40mail.gmail.com Regards, Vignesh
Re: Column Filtering in Logical Replication
Hi Alvaro, Thank you for comments. The patch adds a function get_att_num_by_name; but we have a lsyscache.c > function for that purpose, get_attnum. Maybe that one should be used > instead? > > Thank you for pointing that out, I agree it makes sense to reuse the existing function. Changed it accordingly in the attached patch. > get_tuple_columns_map() returns a bitmapset of the attnos of the columns > in the given list, so its name feels wrong. I propose > get_table_columnset(). However, this function is invoked for every > insert/update change, so it's going to be far too slow to be usable. I > think you need to cache the bitmapset somewhere, so that the function is > only called on first use. I didn't look very closely, but it seems that > struct RelationSyncEntry may be a good place to cache it. > > Makes sense, changed accordingly. > The patch adds a new parse node PublicationTable, but doesn't add > copyfuncs.c, equalfuncs.c, readfuncs.c, outfuncs.c support for it. > Maybe try a compile with WRITE_READ_PARSE_PLAN_TREES and/or > COPY_PARSE_PLAN_TREES enabled to make sure everything is covered. > (I didn't verify that this actually catches anything ...) > I will test this and include these changes in the next version. > The new column in pg_publication_rel is prrel_attr. This name seems at > odds with existing column names (we don't use underscores in catalog > column names). Maybe prattrs is good enough? prrelattrs? We tend to > use plurals for columns that are arrays. > > Renamed it to prattrs as per suggestion. It's not super clear to me that strlist_to_textarray() and related > processing will behave sanely when the column names contain weird > characters such as commas or quotes, or just when used with uppercase > column names. Maybe it's worth having tests that try to break such > cases. > > Sure, I will include these tests in the next version. > You seem to have left a debugging "elog(LOG)" line in OpenTableList. > > Removed. > I got warnings from "git am" about trailing whitespace being added by > the patch in two places. > > Should be fixed now. Thank you, Rahila Syed v1-0001-Add-column-filtering-to-logical-replication.patch Description: Binary data
Re: Introduce pg_receivewal gzip compression tests
‐‐‐ Original Message ‐‐‐
On Monday, July 12th, 2021 at 08:42, Michael Paquier
wrote:
> On Fri, Jul 09, 2021 at 11:26:58AM +, Georgios wrote:
>
> > As suggested on a different thread [1], pg_receivewal can increase it's test
> >
> > coverage. There exists a non trivial amount of code that handles gzip
> >
> > compression. The current patch introduces tests that cover creation of gzip
> >
> > compressed WAL files and the handling of gzip partial segments. Finally the
> >
> > integrity of the compressed files is verified.
>
> - # Verify compressed file's integrity
>
>
> - my $gzip_is_valid = system_log('gzip', '--test', $gzip_wals[0]);
>
>
> - is($gzip_is_valid, 0, "program gzip verified file's integrity");
>
>
>
> libz and gzip are usually split across different packages, hence there
>
> is no guarantee that this command is always available (same comment as
>
> for LZ4 from a couple of days ago).
Of course. Though while going for it, I did find in Makefile.global.in:
TAR = @TAR@
XGETTEXT = @XGETTEXT@
GZIP= gzip
BZIP2 = bzip2
DOWNLOAD = wget -O $@ --no-use-server-timestamps
Which is also used by GNUmakefile.in
distcheck: dist
rm -rf $(dummy)
mkdir $(dummy)
$(GZIP) -d -c $(distdir).tar.gz | $(TAR) xf -
install_prefix=`cd $(dummy) && pwd`; \
This to my understanding means that gzip is expected to exist.
If this is correct, then simply checking for the headers should
suffice, since that is the only dependency for the files to be
created.
If this is wrong, then I will add the discovery code as in the
other patch.
>
> - [
>
>
> - 'pg_receivewal', '-D', $stream_dir, '--verbose',
>
>
> - '--endpos', $nextlsn, '-Z', '5'
>
>
> - ],
>
>
>
> I would keep the compression level to a minimum here, to limit CPU
>
> usage but still compress something faster.
>
> - # Verify compressed file's integrity
>
>
> - my $gzip_is_valid = system_log('gzip', '--test', $gzip_wals[0]);
>
>
> - is($gzip_is_valid, 0, "program gzip verified file's integrity");
>
>
>
> Shouldn't this be coded as a loop going through @gzip_wals?
I would hope that there is only one gz file created. There is a line
further up that tests exactly that.
+ is (scalar(@gzip_wals), 1, "one gzip compressed WAL was created");
Then there should also be a partial gz file which is tested further ahead.
Cheers,
//Georgios
> ---
>
> Michael
Re: Teach pg_receivewal to use lz4 compression
On Mon, Jul 12, 2021 at 11:33 AM wrote: > > > > ‐‐‐ Original Message ‐‐‐ > > On Monday, July 12th, 2021 at 07:56, Michael Paquier > wrote: > > > On Mon, Jul 12, 2021 at 11:10:24AM +0530, Dilip Kumar wrote: > > > > > On Thu, Jul 8, 2021 at 7:48 PM gkokola...@pm.me wrote: > > > > > > > We can, though I am not in favour of doing so. There is seemingly > > > > > > > > little benefit for added complexity. > > > > > > I am really not sure what complexity you are talking about, do you > > > > > > mean since with pglz we were already providing the compression level > > > > > > so let it be as it is but with the new compression method you don't > > > > > > see much benefit of providing compression level or speed? > > > > You mean s/pglz/zlib/ here perhaps? I am not sure what Georgios has > > > > in mind, but my opinion stands on the latter: there is little benefit > > > > in making lz4 faster than the default and reduce compression, as the > > > > default is already a rather low CPU user. > > Thank you all for your comments. I am sitting on the same side as Micheal > on this one. The complexity is not huge, yet there will need to be code to > pass this option to the lz4 api and various test cases to verify for > correctness and integrity. The burden of maintenance of this code vs the > benefit of the option, tilt the scale towards not including the option. +1 for skipping that one, at least for now, and sticking to default-only for lz4. -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Re: Introduce pg_receivewal gzip compression tests
‐‐‐ Original Message ‐‐‐
On Monday, July 12th, 2021 at 11:42, wrote:
> ‐‐‐ Original Message ‐‐‐
>
> On Monday, July 12th, 2021 at 08:42, Michael Paquier mich...@paquier.xyz
> wrote:
>
> > On Fri, Jul 09, 2021 at 11:26:58AM +, Georgios wrote:
> >
> > > As suggested on a different thread [1], pg_receivewal can increase it's
> > > test
> > >
> > > coverage. There exists a non trivial amount of code that handles gzip
> > >
> > > compression. The current patch introduces tests that cover creation of
> > > gzip
> > >
> > > compressed WAL files and the handling of gzip partial segments. Finally
> > > the
> > >
> > > integrity of the compressed files is verified.
> >
> > - # Verify compressed file's integrity
> >
> >
> > - my $gzip_is_valid = system_log('gzip', '--test', $gzip_wals[0]);
> >
> >
> > - is($gzip_is_valid, 0, "program gzip verified file's integrity");
> >
> >
> >
> > libz and gzip are usually split across different packages, hence there
> >
> > is no guarantee that this command is always available (same comment as
> >
> > for LZ4 from a couple of days ago).
>
> Of course. Though while going for it, I did find in Makefile.global.in:
>
> TAR = @TAR@
>
> XGETTEXT = @XGETTEXT@
>
> GZIP = gzip
>
> BZIP2 = bzip2
>
> DOWNLOAD = wget -O $@ --no-use-server-timestamps
>
> Which is also used by GNUmakefile.in
>
> distcheck: dist
>
> rm -rf $(dummy)
>
> mkdir $(dummy)
>
> $(GZIP) -d -c $(distdir).tar.gz | $(TAR) xf -
>
> install_prefix=`cd $(dummy) && pwd`; \
>
> This to my understanding means that gzip is expected to exist.
>
> If this is correct, then simply checking for the headers should
>
> suffice, since that is the only dependency for the files to be
>
> created.
>
> If this is wrong, then I will add the discovery code as in the
>
> other patch.
>
> > - [
> >
> >
> > - 'pg_receivewal', '-D', $stream_dir,
> > '--verbose',
> >
> >
> > - '--endpos', $nextlsn, '-Z', '5'
> >
> >
> > - ],
> >
> >
> >
> > I would keep the compression level to a minimum here, to limit CPU
> >
> > usage but still compress something faster.
> >
> > - # Verify compressed file's integrity
> >
> >
> > - my $gzip_is_valid = system_log('gzip', '--test', $gzip_wals[0]);
> >
> >
> > - is($gzip_is_valid, 0, "program gzip verified file's integrity");
> >
> >
> >
> > Shouldn't this be coded as a loop going through @gzip_wals?
>
> I would hope that there is only one gz file created. There is a line
>
> further up that tests exactly that.
>
> - is (scalar(@gzip_wals), 1, "one gzip compressed WAL was created");
Let me amend that. The line should be instead:
is (scalar(keys @gzip_wals), 1, "one gzip compressed WAL was created");
To properly test that there is one entry.
Let me provide with v2 to fix this.
Cheers,
//Georgios
>
> Then there should also be a partial gz file which is tested further ahead.
>
> Cheers,
>
> //Georgios
>
> > Michael
Re: Introduce pg_receivewal gzip compression tests
‐‐‐ Original Message ‐‐‐
On Monday, July 12th, 2021 at 11:56, wrote:
> ‐‐‐ Original Message ‐‐‐
>
> On Monday, July 12th, 2021 at 11:42, gkokola...@pm.me wrote:
>
> > ‐‐‐ Original Message ‐‐‐
> >
> > On Monday, July 12th, 2021 at 08:42, Michael Paquier mich...@paquier.xyz
> > wrote:
> >
> > > On Fri, Jul 09, 2021 at 11:26:58AM +, Georgios wrote:
> > >
> > > > As suggested on a different thread [1], pg_receivewal can increase it's
> > > > test
> > > >
> > > > coverage. There exists a non trivial amount of code that handles gzip
> > > >
> > > > compression. The current patch introduces tests that cover creation of
> > > > gzip
> > > >
> > > > compressed WAL files and the handling of gzip partial segments. Finally
> > > > the
> > > >
> > > > integrity of the compressed files is verified.
> > >
> > > - # Verify compressed file's integrity
> > >
> > >
> > > - my $gzip_is_valid = system_log('gzip', '--test',
> > > $gzip_wals[0]);
> > >
> > >
> > > - is($gzip_is_valid, 0, "program gzip verified file's
> > > integrity");
> > >
> > >
> > >
> > > libz and gzip are usually split across different packages, hence there
> > >
> > > is no guarantee that this command is always available (same comment as
> > >
> > > for LZ4 from a couple of days ago).
> >
> > Of course. Though while going for it, I did find in Makefile.global.in:
> >
> > TAR = @TAR@
> >
> > XGETTEXT = @XGETTEXT@
> >
> > GZIP = gzip
> >
> > BZIP2 = bzip2
> >
> > DOWNLOAD = wget -O $@ --no-use-server-timestamps
> >
> > Which is also used by GNUmakefile.in
> >
> > distcheck: dist
> >
> > rm -rf $(dummy)
> >
> > mkdir $(dummy)
> >
> > $(GZIP) -d -c $(distdir).tar.gz | $(TAR) xf -
> >
> > install_prefix=`cd $(dummy) && pwd`; \
> >
> > This to my understanding means that gzip is expected to exist.
> >
> > If this is correct, then simply checking for the headers should
> >
> > suffice, since that is the only dependency for the files to be
> >
> > created.
> >
> > If this is wrong, then I will add the discovery code as in the
> >
> > other patch.
> >
> > > - [
> > >
> > >
> > > - 'pg_receivewal', '-D', $stream_dir,
> > > '--verbose',
> > >
> > >
> > > - '--endpos', $nextlsn, '-Z', '5'
> > >
> > >
> > > - ],
> > >
> > >
> > >
> > > I would keep the compression level to a minimum here, to limit CPU
> > >
> > > usage but still compress something faster.
> > >
> > > - # Verify compressed file's integrity
> > >
> > >
> > > - my $gzip_is_valid = system_log('gzip', '--test',
> > > $gzip_wals[0]);
> > >
> > >
> > > - is($gzip_is_valid, 0, "program gzip verified file's
> > > integrity");
> > >
> > >
> > >
> > > Shouldn't this be coded as a loop going through @gzip_wals?
> >
> > I would hope that there is only one gz file created. There is a line
> >
> > further up that tests exactly that.
> >
> > - is (scalar(@gzip_wals), 1, "one gzip compressed WAL was created");
>
> Let me amend that. The line should be instead:
>
> is (scalar(keys @gzip_wals), 1, "one gzip compressed WAL was created");
>
> To properly test that there is one entry.
>
> Let me provide with v2 to fix this.
Please find v2 attached with the above.
Cheers,
//Georgios
>
> Cheers,
>
> //Georgios
>
> > Then there should also be a partial gz file which is tested further
> > ahead.
> >
> > Cheers,
> >
> > //Georgios
> >
> >
> > > Michael
v2-0001-Introduce-pg_receivewal-gzip-compression-tests.patch
Description: Binary data
Re: Teach pg_receivewal to use lz4 compression
On Mon, Jul 12, 2021 at 3:15 PM Magnus Hagander wrote: > > On Mon, Jul 12, 2021 at 11:33 AM wrote: > > > > > > > > ‐‐‐ Original Message ‐‐‐ > > > > On Monday, July 12th, 2021 at 07:56, Michael Paquier > > wrote: > > > > > On Mon, Jul 12, 2021 at 11:10:24AM +0530, Dilip Kumar wrote: > > > > > > > On Thu, Jul 8, 2021 at 7:48 PM gkokola...@pm.me wrote: > > > > > > > > > We can, though I am not in favour of doing so. There is seemingly > > > > > > > > > > little benefit for added complexity. > > > > > > > > I am really not sure what complexity you are talking about, do you > > > > > > > > mean since with pglz we were already providing the compression level > > > > > > > > so let it be as it is but with the new compression method you don't > > > > > > > > see much benefit of providing compression level or speed? > > > > > > You mean s/pglz/zlib/ here perhaps? I am not sure what Georgios has > > > > > > in mind, but my opinion stands on the latter: there is little benefit > > > > > > in making lz4 faster than the default and reduce compression, as the > > > > > > default is already a rather low CPU user. > > > > Thank you all for your comments. I am sitting on the same side as Micheal > > on this one. The complexity is not huge, yet there will need to be code to > > pass this option to the lz4 api and various test cases to verify for > > correctness and integrity. The burden of maintenance of this code vs the > > benefit of the option, tilt the scale towards not including the option. > > +1 for skipping that one, at least for now, and sticking to > default-only for lz4. Okay, fine with me as well. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: Introduce pg_receivewal gzip compression tests
Le 12/07/2021 à 12:27, gkokola...@pm.me a écrit : Shouldn't this be coded as a loop going through @gzip_wals? >>> I would hope that there is only one gz file created. There is a line >>> >>> further up that tests exactly that. >>> >>> - is (scalar(@gzip_wals), 1, "one gzip compressed WAL was created"); >> Let me amend that. The line should be instead: >> >> is (scalar(keys @gzip_wals), 1, "one gzip compressed WAL was created"); >> >> To properly test that there is one entry. >> >> Let me provide with v2 to fix this. The following tests are not correct in Perl even if Perl returns the right value. + is (scalar(keys @gzip_wals), 1, "one gzip compressed WAL was created"); + is (scalar(keys @gzip_partial_wals), 1, + "one partial gzip compressed WAL was created"); Function keys or values are used only with hashes but here you are using arrays. To obtain the length of the array you can just use the scalar function as Perl returns the length of the array when it is called in a scalar context. Please use the following instead: + is (scalar(@gzip_wals), 1, "one gzip compressed WAL was created"); + is (scalar(@gzip_partial_wals), 1, + "one partial gzip compressed WAL was created"); -- Gilles Darold http://www.darold.net/
Re: Introduce pg_receivewal gzip compression tests
On Mon, Jul 12, 2021 at 09:42:32AM +, gkokola...@pm.me wrote:
> This to my understanding means that gzip is expected to exist.
> If this is correct, then simply checking for the headers should
> suffice, since that is the only dependency for the files to be
> created.
You cannot expect this to work on Windows when it comes to MSVC for
example, as gzip may not be in the environment PATH so the test would
fail hard. Let's just rely on $ENV{GZIP} instead, and skip the test
if it is not defined.
--
Michael
signature.asc
Description: PGP signature
Re: pg_stats and range statistics
Hi,
thanks for the review and corrections.
On 11.07.2021 21:54, Soumyadeep Chakraborty wrote:
Hello,
This should have been added with [1].
Excerpt from the documentation:
"pg_stats is also designed to present the information in a more readable
format than the underlying catalog — at the cost that its schema must
be extended whenever new slot types are defined for pg_statistic." [2]
So, I added a reminder in pg_statistic.h.
Good point.
Attached is v2 of this patch with some cosmetic changes.
I wonder why "TODO: catalog version bump"? This patch doesn't change
catalog structure, or I miss something?
Renamed the columns a
bit and updated the docs to be a bit more descriptive.
(range_length_empty_frac -> empty_range_frac, range_bounds_histogram ->
range_bounds_histograms)
I intended to make the same prefix ("range_") for all columns concerned
with range types, although I'm fine with the proposed naming.
One question:
We do have the option of representing the histogram of lower bounds separately
from the histogram of upper bounds, as two separate view columns. Don't know if
there is much utility though and there is a fair bit of added complexity: see
below. Thoughts?
I thought about it too, and decided not to transform the underlying data
structure. As far as I can see, pg_stats never employed such
transformations. For example, STATISTIC_KIND_DECHIST is an array
containing the histogram followed by the average in its last element. It
is shown in pg_stats.elem_count_histogram as is, although it arguably
may be splitted into two fields. All in all, I believe pg_stats's job is
to "unpack" stavalues and stanumbers into meaningful fields, and not to
try to go deeper than that.
My attempts via SQL (unnest -> lower|upper -> array_agg) were futile given
unnest does not play nice with anyarray. For instance:
select unnest(stavalues1) from pg_statistic;
ERROR: cannot determine element type of "anyarray" argument
Maybe the only option is to write a UDF pg_get_{lower|upper}_bounds_histogram
which can do something similar to what calc_hist_selectivity does:
/*
* Convert histogram of ranges into histograms of its lower and upper
* bounds.
*/
nhist = hslot.nvalues;
hist_lower = (RangeBound *) palloc(sizeof(RangeBound) * nhist);
hist_upper = (RangeBound *) palloc(sizeof(RangeBound) * nhist);
for (i = 0; i < nhist; i++)
{
bool empty;
range_deserialize(rng_typcache, DatumGetRangeTypeP(hslot.values[i]),
&hist_lower[i], &hist_upper[i], &empty);
/* The histogram should not contain any empty ranges */
if (empty)
elog(ERROR, "bounds histogram contains an empty range");
}
This is looking good and ready.
[1]
https://github.com/postgres/postgres/commit/918eee0c497c88260a2e107318843c9b1947bc6f
[2] https://www.postgresql.org/docs/devel/view-pg-stats.html
Regards,
Soumyadeep (VMware)
Fix comments of heap_prune_chain()
Hi,While I’m reading source codes related to vacuum, I found comments whichdon’t seem to fit the reality. I think the commit[1] just forgot to fix them.What do you think?[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=dc7420c2c9274a283779ec19718d2d16323640c0 Regards,-- Masahiro IkedaNTT DATA CORPORATION v1-0001-fix_heap_prune_chain.patch Description: Binary data
Re: Support kerberos authentication for postgres_fdw
>But in this case, what dose Kerberos give over just using a password >based solution? It adds complexity, but what's teh actual gain? That's due to policy of some customers. They require all login to be kerberos based and password-less. I suppose this way they don't need to maintain passwords in each database and the same keytab file may be used in connections to multiple databases. If we can do the delegation approach right, it's clearly a superior solution since keytab file management is also quite heavy burden.
[PATCH] improve the pg_upgrade error message
While looking into one of the pg_upgrade issue, I found it challenging to find out the database that has the datallowconn set to 'false' that was throwing following error: *"All non-template0 databases must allow connections, i.e. their pg_database.datallowconn must be true"* edb=# create database mydb; CREATE DATABASE edb=# update pg_database set datallowconn='false' where datname like 'mydb'; UPDATE 1 Now, when I try to upgrade the server, without the patch we get above error, which leaves no clue behind about which database has datallowconn set to 'false'. It can be argued that we can query the pg_database catalog and find that out easily, but at times it is challenging to get that from the customer environment. But, anyways I feel we have scope to improve the error message here per the attached patch. With attached patch, now I get following error: *"All non-template0 databases must allow connections, i.e. their pg_database.datallowconn must be true; database "mydb" has datallowconn set to false."* Regards, Jeevan Ladhe 0001-Improve-the-pg_upgrade-error-message.patch Description: Binary data
Re: row filtering for logical replication
While looking at the other logrep patch [1] (column filtering) I noticed Alvaro's comment regarding a new parsenode (PublicationTable) not having read/out/equal/copy funcs. I'd bet the same thing applies here, so perhaps see if the patch needs the same fix. [1] https://www.postgresql.org/message-id/202107062342.eq6htmp2wgp2%40alvherre.pgsql -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Skipping logical replication transactions on subscriber side
On Mon, Jul 12, 2021 at 11:13 AM Masahiko Sawada wrote: > > On Mon, Jul 12, 2021 at 1:15 PM Amit Kapila wrote: > > > > On Mon, Jul 12, 2021 at 9:37 AM Alexey Lesovsky wrote: > > > > > > On Mon, Jul 12, 2021 at 8:36 AM Amit Kapila > > > wrote: > > >> > > >> > > > >> > Ok, looks nice. But I am curious how this will work in the case when > > >> > there are two (or more) errors in the same subscription, but different > > >> > relations? > > >> > > > >> > > >> We can't proceed unless the first error is resolved, so there > > >> shouldn't be multiple unresolved errors. > > > > > > > > > Ok. I thought multiple errors are possible when many tables are > > > initialized using parallel workers (with > > > max_sync_workers_per_subscription > 1). > > > > > > > Yeah, that is possible but that covers under the second condition > > mentioned by me and in such cases I think we should have separate rows > > for each tablesync. Is that right, Sawada-san or do you have something > > else in mind? > > Yeah, I agree to have separate rows for each table sync. The table > should not be processed by both the table sync worker and the apply > worker at a time so the pair of subscription OID and relation OID will > be unique. I think that we have a boolean column in the view, > indicating whether the error entry is reported by the table sync > worker or the apply worker, or maybe we also can have the action > column show "TABLE SYNC" if the error is reported by the table sync > worker. > Or similar to backend_type (text) in pg_stat_activity, we can have something like error_source (text) which will display apply worker or tablesync worker? I think if we have this column then even if there is a chance that both apply and sync worker operates on the same relation, we can identify it via this column. -- With Regards, Amit Kapila.
Re: Diagnostic comment in LogicalIncreaseXminForSlot
On Mon, Jul 12, 2021 at 8:39 AM Amit Kapila wrote:
> On Mon, Jul 5, 2021 at 12:54 PM Masahiko Sawada
> wrote:
> >
> > On Fri, May 21, 2021 at 6:00 PM Ashutosh Bapat
> > wrote:
> > >
> > > It's there in CF. I am fine with PG-15. It will be good to patch the
> back-branches to have this extra diagnostic information available.
> >
> > The patch looks to me.
> >
>
> {
> slot->candidate_catalog_xmin = xmin;
> slot->candidate_xmin_lsn = current_lsn;
> + elog(DEBUG1, "got new catalog_xmin %u at %X/%X", xmin,
> + LSN_FORMAT_ARGS(current_lsn));
> }
> SpinLockRelease(&slot->mutex);
>
> Today, again looking at this patch, I don't think doing elog inside
> spinlock is a good idea. We can either release spinlock before it or
> use some variable to remember that we need to write such an elog and
> do it after releasing the lock. What do you think?
The elog will be effective only under DEBUG1, otherwise it will be almost a
NOOP. I am wondering whether it's worth adding a bool assignment and move
the elog out only for DEBUG1. Anyway, will defer it to you.
> I have noticed that
> a nearby function LogicalIncreaseRestartDecodingForSlot() logs similar
> information after releasing spinlock, so it is better to follow the
> same here as well.
>
Now that you mention it, the code their looks rather suspicious :)
We acquire the spinlock at the beginning of the function but do not release
it if (restart_lsn <= slot->data.restart_lsn) or if (current_lsn <=
slot->data.confirmed_flush). I might be missing something there. But it
looks unrelated.
--
--
Best Wishes,
Ashutosh
2021-07 CF now in progress
Hackers, The Commitfest 2021-07 is now in progress. It is one of the biggest one. Total number of patches of this commitfest is 342. Needs review: 204. Waiting on Author: 40. Ready for Committer: 18. Committed: 57. Moved to next CF: 3. Withdrawn: 15. Rejected: 3. Returned with Feedback: 2. Total: 342. If you are a patch author, please check http://commitfest.cputube.org to be sure your patch still applies, compiles, and passes tests. We need your involvement and participation in reviewing the patches. Let's try and make this happen. -- Regards. Ibrar Ahmed
Re: Add proper planner support for ORDER BY / DISTINCT aggregates
On Sun, 13 Jun 2021 at 03:07, David Rowley wrote: > > Please find attached my WIP patch. It's WIP due to what I mentioned > in the above paragraph and also because I've not bothered to add JIT > support for the new expression evaluation steps. I've split this patch into two parts. 0001 Adds planner support for ORDER BY aggregates. 0002 is a WIP patch for DISTINCT support. This still lacks JIT support and I'm still not certain of the best where to store the previous value or tuple to determine if the current one is distinct from it. The 0001 patch is fairly simple and does not require much in the way of changes in the planner aside from standard_qp_callback(). Surprisingly the executor does not need a great deal of work here either. It's just mostly about skipping the normal agg(.. ORDER BY) code when the Aggref is presorted. David v2-0001-Add-planner-support-for-ORDER-BY-aggregates.patch Description: Binary data v2-0002-WIP-Add-planner-support-for-DISTINCT-aggregates.patch Description: Binary data
Re: [PATCH] improve the pg_upgrade error message
On Mon, 2021-07-12 at 16:58 +0530, Jeevan Ladhe wrote: > While looking into one of the pg_upgrade issue, I found it > challenging to find out the database that has the datallowconn set to > 'false' that was throwing following error: > > "All non-template0 databases must allow connections, i.e. their > pg_database.datallowconn must be true" > > It can be argued that we can query the pg_database > catalog and find that out easily, but at times it is challenging to get > that from the customer environment. > > With attached patch, now I get following error: > "All non-template0 databases must allow connections, i.e. their > pg_database.datallowconn must be true; database "mydb" has datallowconn set > to false." I am in favor of that in principle, but I think that additional information should be separate line. Yours, Laurenz Albe
Re: ATTACH PARTITION locking documentation for DEFAULT partitions
On Mon, 5 Jul 2021 at 01:01, David Rowley wrote: > I've spent a bit of time hacking at this and I've come up with the > attached patch. Matthias, any thoughts on my revised version of the patch? David
Re: [PATCH] improve the pg_upgrade error message
+1 for the change. Patch looks good to me. On Mon, Jul 12, 2021 at 4:59 PM Jeevan Ladhe wrote: > While looking into one of the pg_upgrade issue, I found it > > challenging to find out the database that has the datallowconn set to > > 'false' that was throwing following error: > > > *"All non-template0 databases must allow connections, i.e. their > pg_database.datallowconn must be true"* > > > edb=# create database mydb; > > CREATE DATABASE > > edb=# update pg_database set datallowconn='false' where datname like > 'mydb'; > > UPDATE 1 > > > Now, when I try to upgrade the server, without the patch we get above > > error, which leaves no clue behind about which database has datallowconn > > set to 'false'. It can be argued that we can query the pg_database > > catalog and find that out easily, but at times it is challenging to get > > that from the customer environment. But, anyways I feel we have scope to > > improve the error message here per the attached patch. > > > With attached patch, now I get following error: > > *"All non-template0 databases must allow connections, i.e. their > pg_database.datallowconn must be true; database "mydb" has datallowconn set > to false."* > > > > Regards, > > Jeevan Ladhe > -- -- Thanks & Regards, Suraj kharage, edbpostgres.com
Re: ATTACH PARTITION locking documentation for DEFAULT partitions
On Mon, 12 Jul 2021 at 14:06, David Rowley wrote: > > On Mon, 5 Jul 2021 at 01:01, David Rowley wrote: > > I've spent a bit of time hacking at this and I've come up with the > > attached patch. > > Matthias, any thoughts on my revised version of the patch? Sorry for the delay. I think that covers the basics of what I was missing in these docs, and although it does not cover the recursive 'if the check is implied by constraints don't lock this partition', I'd say that your suggested patch is good enough. Thanks for looking over this. Kind regards, Matthias van de Meent
Re: PostmasterIsAlive() in recovery (non-USE_POST_MASTER_DEATH_SIGNAL builds)
On Fri, Jun 11, 2021 at 1:18 PM Tom Lane wrote:
> Heikki Linnakangas writes:
> > On 09/04/2021 07:01, Thomas Munro wrote:
> >> This seems to work on Linux, macOS, FreeBSD and OpenBSD (and I assume
> >> any other BSD). Can anyone tell me if it works on illumos, AIX or
> >> HPUX, and if not, how to fix it or disable the feature gracefully?
> >> For now the patch assumes that if you have SIGIO then you can do this;
> >> perhaps it should also test for O_ASYNC? Perhaps HPUX has the signal
> >> but requires a different incantation with I_SETSIG?
>
> I took a look on HPUX 10.20 (gaur's host):
Thanks both for looking!
Unfortunately I'll have to withdraw this patch in its current form.
On closer inspection, only the last backend to start up and do
F_SETOWN on the pipe receives the signal. We'd probably have to
create a separate pipe for each backend, or something like that, which
seems unwarranted so far.
> * I found I_SETSIG, but ...
>
> $ grep -r SETSIG /usr/include
> /usr/include/sys/stropts.h:#define I_SETSIG_IO('S', 9) /*
> request SIGPOLL signal on events */
>
> stropts.h seems to be for a feature called "streams", which is
> probably nonstandard enough that we don't want to deal with it.
Agreed. It is technically POSIX, but optional and marked obsolescent.
It's annoying to see that I_SETSIG did allow multiple processes to
register to receive signals for the same event on the same underlying
stream, unlike F_SETOWN. Oh well.
Re: [PATCH] improve the pg_upgrade error message
On Mon, Jul 12, 2021 at 02:06:31PM +0200, Laurenz Albe wrote: > On Mon, 2021-07-12 at 16:58 +0530, Jeevan Ladhe wrote: > > While looking into one of the pg_upgrade issue, I found it > > challenging to find out the database that has the datallowconn set to > > 'false' that was throwing following error: > > > > "All non-template0 databases must allow connections, i.e. their > > pg_database.datallowconn must be true" > > > > It can be argued that we can query the pg_database > > catalog and find that out easily, but at times it is challenging to get > > that from the customer environment. > > > > With attached patch, now I get following error: > > "All non-template0 databases must allow connections, i.e. their > > pg_database.datallowconn must be true; database "mydb" has datallowconn set > > to false." > > I am in favor of that in principle, but I think that additional information > should be separate line. I think the style for this kind of error is established by commit 1634d3615. This shows "In database: ..." More importantly, if you're going to show the name of the problematic DB, you should show the name of *all* the problem DBs. Otherwise it gives the impression the upgrade will progress if you fix just that one. The admin might fix DB123, restart their upgrade procedure, spend 5 or 15 minutes with that, only to have it then fail on DB1234. -- Justin
Re: refactoring basebackup.c
On 7/8/21 9:26 PM, Robert Haas wrote: Here at last is a new version. Please refer this scenario ,where backup target using --server-compression is closing the server unexpectedly if we don't provide -no-manifest option [tushar@localhost bin]$ ./pg_basebackup --server-compression=gzip4 -t server:/tmp/data_1 -Xnone NOTICE: WAL archiving is not enabled; you must ensure that all required WAL segments are copied through other means to complete the backup pg_basebackup: error: could not read COPY data: server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. if we try to check with -Ft then this same scenario is working ? [tushar@localhost bin]$ ./pg_basebackup --server-compression=gzip4 -Ft -D data_0 -Xnone NOTICE: WAL archiving is not enabled; you must ensure that all required WAL segments are copied through other means to complete the backup [tushar@localhost bin]$ -- regards,tushar EnterpriseDB https://www.enterprisedb.com/ The Enterprise PostgreSQL Company
Re: Column Filtering in Logical Replication
On 7/12/21 10:32 AM, Rahila Syed wrote: > Hi Peter, > > Hi, I was wondering if/when a subset of cols is specified then does > that mean it will be possible for the table to be replicated to a > *smaller* table at the subscriber side? > > e.g Can a table with 7 cols replicated to a table with 2 cols? > > table tab1(a,b,c,d,e,f,g) --> CREATE PUBLICATION pub1 FOR TABLE > tab1(a,b) --> table tab1(a,b) > > ~~ > > > I thought maybe that should be possible, but the expected behaviour > for that scenario was not very clear to me from the thread/patch > comments. And the new TAP test uses the tab1 table created exactly the > same for pub/sub, so I couldn't tell from the test code either. > > > Currently, this capability is not included in the patch. If the table on > the subscriber > server has lesser attributes than that on the publisher server, it > throws an error at the > time of CREATE SUBSCRIPTION. > That's a bit surprising, to be honest. I do understand the patch simply treats the filtered columns as "unchanged" because that's the simplest way to filter the *data* of the columns. But if someone told me we can "filter columns" I'd expect this to work without the columns on the subscriber. > About having such a functionality, I don't immediately see any issue > with it as long > as we make sure replica identity columns are always present on both > instances. Yeah, that seems like an inherent requirement. > However, need to carefully consider situations in which a server > subscribes to multiple > publications, each publishing a different subset of columns of a table. > Isn't that pretty much the same situation as for multiple subscriptions each with a different set of I/U/D operations? IIRC we simply merge those, so why not to do the same thing here and merge the attributes? regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Unused function parameter in get_qual_from_partbound()
On Tue, Jun 8, 2021 at 10:50 PM Michael Paquier wrote: > > At first glance, this looked to me like breaking something just for > sake of breaking it, but removing the rel argument could be helpful > to simplify any external code calling it as there would be no need for > this extra Relation. So that looks like a good idea, no need to rush > that into 14 though. I found no external references in codesearch.debian.net. I plan to commit this in the next couple of days unless there are objections. -- John Naylor EDB: http://www.enterprisedb.com
Re: Diagnostic comment in LogicalIncreaseXminForSlot
On Mon, Jul 12, 2021 at 5:28 PM Ashutosh Bapat wrote: > > On Mon, Jul 12, 2021 at 8:39 AM Amit Kapila wrote: >> >> I have noticed that >> a nearby function LogicalIncreaseRestartDecodingForSlot() logs similar >> information after releasing spinlock, so it is better to follow the >> same here as well. > > > Now that you mention it, the code their looks rather suspicious :) > We acquire the spinlock at the beginning of the function but do not release > it if (restart_lsn <= slot->data.restart_lsn) or if (current_lsn <= > slot->data.confirmed_flush). > Note that we end else if with (current_lsn <= slot->data.confirmed_flush) and after that there is a new if. We release lock in both the if/else conditions, so the code is fine as it is. -- With Regards, Amit Kapila.
Re: Column Filtering in Logical Replication
On 7/12/21 11:38 AM, Rahila Syed wrote: > Hi Alvaro, > > Thank you for comments. > > The patch adds a function get_att_num_by_name; but we have a lsyscache.c > function for that purpose, get_attnum. Maybe that one should be used > instead? > > Thank you for pointing that out, I agree it makes sense to reuse the > existing function. > Changed it accordingly in the attached patch. > > > get_tuple_columns_map() returns a bitmapset of the attnos of the columns > in the given list, so its name feels wrong. I propose > get_table_columnset(). However, this function is invoked for every > insert/update change, so it's going to be far too slow to be usable. I > think you need to cache the bitmapset somewhere, so that the function is > only called on first use. I didn't look very closely, but it seems that > struct RelationSyncEntry may be a good place to cache it. > > Makes sense, changed accordingly. > To nitpick, I find "Bitmapset *att_list" a bit annoying, because it's not really a list ;-) FWIW "make check" fails for me with this version, due to segfault in OpenTableLists. Apparenly there's some confusion - the code expects the list to contain PublicationTable nodes, and tries to extract the RangeVar from the elements. But the list actually contains RangeVar, so this crashes and burns. See the attached backtrace. I'd bet this is because the patch uses list of RangeVar in some cases and list of PublicationTable in some cases, similarly to the "row filtering" patch nearby. IMHO this is just confusing and we should always pass list of PublicationTable nodes. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company Core was generated by `postgres: user regression [local] ALTER PUBLICATION '. Program terminated with signal SIGSEGV, Segmentation fault. #0 0x006be5b5 in OpenTableList (tables=0x27b35a0) at publicationcmds.c:520 520 boolrecurse = rv->inh; Missing separate debuginfos, use: dnf debuginfo-install glibc-2.32-6.fc33.x86_64 (gdb) bt #0 0x006be5b5 in OpenTableList (tables=0x27b35a0) at publicationcmds.c:520 #1 0x006be137 in AlterPublicationTables (stmt=0x27b35f8, rel=0x73e26485ec80, tup=0x2877c98) at publicationcmds.c:375 #2 0x006be458 in AlterPublication (stmt=0x27b35f8) at publicationcmds.c:464 #3 0x009762b4 in ProcessUtilitySlow (pstate=0x2877b80, pstmt=0x27b3968, queryString=0x27b2ae0 "ALTER PUBLICATION testpub_default SET TABLE testpub_tbl1;", context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x27b3a58, qc=0x7ffec211be70) at utility.c:1809 #4 0x00974968 in standard_ProcessUtility (pstmt=0x27b3968, queryString=0x27b2ae0 "ALTER PUBLICATION testpub_default SET TABLE testpub_tbl1;", readOnlyTree=false, context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x27b3a58, qc=0x7ffec211be70) at utility.c:1049 #5 0x00973bb8 in ProcessUtility (pstmt=0x27b3968, queryString=0x27b2ae0 "ALTER PUBLICATION testpub_default SET TABLE testpub_tbl1;", readOnlyTree=false, context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x27b3a58, qc=0x7ffec211be70) at utility.c:527 #6 0x00972890 in PortalRunUtility (portal=0x2816ff0, pstmt=0x27b3968, isTopLevel=true, setHoldSnapshot=false, dest=0x27b3a58, qc=0x7ffec211be70) at pquery.c:1147 #7 0x00972af4 in PortalRunMulti (portal=0x2816ff0, isTopLevel=true, setHoldSnapshot=false, dest=0x27b3a58, altdest=0x27b3a58, qc=0x7ffec211be70) at pquery.c:1304 #8 0x0097202d in PortalRun (portal=0x2816ff0, count=9223372036854775807, isTopLevel=true, run_once=true, dest=0x27b3a58, altdest=0x27b3a58, qc=0x7ffec211be70) at pquery.c:786 #9 0x0096bc4f in exec_simple_query (query_string=0x27b2ae0 "ALTER PUBLICATION testpub_default SET TABLE testpub_tbl1;") at postgres.c:1214 #10 0x0097015a in PostgresMain (argc=1, argv=0x7ffec211c100, dbname=0x27deed8 "regression", username=0x27deeb8 "user") at postgres.c:4486 #11 0x008ad08d in BackendRun (port=0x27d7fc0) at postmaster.c:4507 #12 0x008aca0c in BackendStartup (port=0x27d7fc0) at postmaster.c:4229 #13 0x008a8fc0 in ServerLoop () at postmaster.c:1745 #14 0x008a888b in PostmasterMain (argc=8, argv=0x27ac590) at postmaster.c:1417 #15 0x007ab2b0 in main (argc=8, argv=0x27ac590) at main.c:209
Re: pg_stats and range statistics
On 7/12/21 1:10 PM, Egor Rogov wrote:
> Hi,
>
> thanks for the review and corrections.
>
> On 11.07.2021 21:54, Soumyadeep Chakraborty wrote:
>> Hello,
>>
>> This should have been added with [1].
>>
>> Excerpt from the documentation:
>> "pg_stats is also designed to present the information in a more readable
>> format than the underlying catalog — at the cost that its schema must
>> be extended whenever new slot types are defined for pg_statistic." [2]
>>
>> So, I added a reminder in pg_statistic.h.
>
> Good point.
>
>
>> Attached is v2 of this patch with some cosmetic changes.
>
> I wonder why "TODO: catalog version bump"? This patch doesn't change
> catalog structure, or I miss something?
>
It changes system_views.sql, which is catalog change, as it redefines
the pg_stats system view (it adds 3 more columns). So it changes what
you get after initdb, hence catversion has to be bumped.
>
>> Renamed the columns a
>> bit and updated the docs to be a bit more descriptive.
>> (range_length_empty_frac -> empty_range_frac, range_bounds_histogram ->
>> range_bounds_histograms)
>
> I intended to make the same prefix ("range_") for all columns concerned
> with range types, although I'm fine with the proposed naming.
>
Yeah, I'd vote to change empty_range_frac -> range_empty_frac.
>
>> One question:
>>
>> We do have the option of representing the histogram of lower bounds
>> separately
>> from the histogram of upper bounds, as two separate view columns.
>> Don't know if
>> there is much utility though and there is a fair bit of added
>> complexity: see
>> below. Thoughts?
>
> I thought about it too, and decided not to transform the underlying data
> structure. As far as I can see, pg_stats never employed such
> transformations. For example, STATISTIC_KIND_DECHIST is an array
> containing the histogram followed by the average in its last element. It
> is shown in pg_stats.elem_count_histogram as is, although it arguably
> may be splitted into two fields. All in all, I believe pg_stats's job is
> to "unpack" stavalues and stanumbers into meaningful fields, and not to
> try to go deeper than that.
>
Not firm opinion, but the pg_stats is meant to be easier to
read/understand for humans. So far the transformation were simple
because all the data was fairly simple, but the range stuff may need
more complex transformation.
For example we do quite a bit more in pg_stats_ext views, because it
deals with multi-column stats.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: [PATCH] Use optimized single-datum tuplesort in ExecSort
On Wed, 7 Jul 2021 at 21:32, Ronan Dunklau wrote:
> In the meantime I fixed some formatting issues, please find attached a new
> patch.
I started to look at this.
First I wondered how often we might be able to apply this
optimisation, so I ran make check after adding some elog(NOTICE) calls
to output which method is going to be used just before we do the
tuplestore_begin_* calls. It looks like there are 614 instances of
Datum sorts and 4223 of tuple sorts. That's about 14.5% datum sorts.
223 of the 614 are byval types and the other 391 are byref. Not that
the regression tests are a good reflection of the real world, but if
it were then that's quite a good number of cases to be able to
optimise.
As for the patch, just a few things:
1. Can you add the missing braces in this if condition and the else
condition that belongs to it.
+ if (node->is_single_val)
+ for (;;)
+ {
2. I think it would nicer to name the new is_single_val field
"datumSort" instead. To me it seems more clear what it is for.
3. This seems to be a bug fix where byval datum sorts do not properly
handle bounded sorts. I think that maybe that should be fixed and
backpatched. I don't see anything that says Datum sorts can't be
bounded and if there were some restriction on that I'd expect
tuplesort_set_bound() to fail when the Tuplesortstate had been set up
with tuplesort_begin_datum().
static void
free_sort_tuple(Tuplesortstate *state, SortTuple *stup)
{
- FREEMEM(state, GetMemoryChunkSpace(stup->tuple));
- pfree(stup->tuple);
+ /*
+ * If the SortTuple is actually only a single Datum, which was not copied
+ * as it is a byval type, do not try to free it nor account for it in
+ * memory used.
+ */
+ if (stup->tuple)
+ {
+ FREEMEM(state, GetMemoryChunkSpace(stup->tuple));
+ pfree(stup->tuple);
+ }
I can take this to another thread.
That's all I have for now.
David
Is tuplesort meant to support bounded datum sorts?
Over on [1], Ronan is working on allowing Datum sorts for nodeSort.c
when we're just sorting a single Datum.
I was looking at his v4 patch and noticed that he'd modified
free_sort_tuple() to conditionally only free the sort tuple if it's
non-NULL. Without this change, the select.sql regression test fails
on:
select * from onek,
(values ((select i from
(values(1), (2), (389), (1000), (2000), ((select 10029))) as foo(i)
order by i asc limit 1))) bar (i)
where onek.unique1 = bar.i;
The limit 1 makes this a bounded sort and we call free_sort_tuple()
during make_bounded_heap().
It looks like this has likely never come up before because the only
time we use tuplesort_set_bound() is in nodeSort.c and
nodeIncrementalSort.c, none of those currently use datum sorts.
However, I'm thinking this is still a bug that should be fixed
separately from Ronan's main patch.
Does anyone else have thoughts on this?
The fragment in question is:
@@ -4773,6 +4773,14 @@ leader_takeover_tapes(Tuplesortstate *state)
static void
free_sort_tuple(Tuplesortstate *state, SortTuple *stup)
{
- FREEMEM(state, GetMemoryChunkSpace(stup->tuple));
- pfree(stup->tuple);
+/*
+ * If the SortTuple is actually only a single Datum, which was not copied
+ * as it is a byval type, do not try to free it nor account for it in
+ * memory used.
+ */
+ if (stup->tuple)
+ {
+ FREEMEM(state, GetMemoryChunkSpace(stup->tuple));
+ pfree(stup->tuple);
+ }
}
David
[1] https://www.postgresql.org/message-id/3060002.hb0XKQ11pn@aivenronan
Re: ATTACH PARTITION locking documentation for DEFAULT partitions
On Tue, 13 Jul 2021 at 00:14, Matthias van de Meent wrote: > Sorry for the delay. I think that covers the basics of what I was > missing in these docs, and although it does not cover the recursive > 'if the check is implied by constraints don't lock this partition', > I'd say that your suggested patch is good enough. Thanks for looking > over this. Isn't that covered the following? + + Further locks must also be held on all sub-partitions if the table being + attached is itself a partitioned table. Likewise if the default + partition is itself a partitioned table. The locking of the + sub-partitions can be avoided by adding a CHECK + constraint as described in + . David
Re: enable_resultcache confusion
David Rowley writes: > On Mon, 12 Jul 2021 at 03:22, Justin Pryzby wrote: >> |This is useful if only a small percentage of rows is checked on >> |the inner side and is controlled by > |linkend="guc-enable-resultcache"/>. > You might be right there, but I'm not too sure if I changed that that > it might cause a mention of the rename to be missed in the changes > since beta2 notes. You need to change it, because IIUC that will be a dangling cross-reference, causing the v14 docs to fail to build at all. > Additionally, I was unsure about touching typedefs.list. In the patch > I changed it, but wasn't too sure if that was the correct thing to do. > In normal circumstances, i.e writing new code, I'd not touch it. I'd suggest replacing it in typedefs.list, since there is unlikely to be any further update to v14's copy otherwise, and even in HEAD I'm not sure it'd get updated before we approach the v15 branch. regards, tom lane
Re: [PATCH] Use optimized single-datum tuplesort in ExecSort
Le lundi 12 juillet 2021, 15:11:17 CEST David Rowley a écrit :
> On Wed, 7 Jul 2021 at 21:32, Ronan Dunklau wrote:
> > In the meantime I fixed some formatting issues, please find attached a new
> > patch.
>
> I started to look at this.
Thank you ! I'm attaching a new version of the patch taking your remarks into
account.
>
> First I wondered how often we might be able to apply this
> optimisation, so I ran make check after adding some elog(NOTICE) calls
> to output which method is going to be used just before we do the
> tuplestore_begin_* calls. It looks like there are 614 instances of
> Datum sorts and 4223 of tuple sorts. That's about 14.5% datum sorts.
> 223 of the 614 are byval types and the other 391 are byref. Not that
> the regression tests are a good reflection of the real world, but if
> it were then that's quite a good number of cases to be able to
> optimise.
That's an interesting stat.
>
> As for the patch, just a few things:
>
> 1. Can you add the missing braces in this if condition and the else
> condition that belongs to it.
>
> + if (node->is_single_val)
> + for (;;)
> + {
>
Done.
> 2. I think it would nicer to name the new is_single_val field
> "datumSort" instead. To me it seems more clear what it is for.
Done.
>
> 3. This seems to be a bug fix where byval datum sorts do not properly
> handle bounded sorts. I think that maybe that should be fixed and
> backpatched. I don't see anything that says Datum sorts can't be
> bounded and if there were some restriction on that I'd expect
> tuplesort_set_bound() to fail when the Tuplesortstate had been set up
> with tuplesort_begin_datum().
I've kept this as-is for now, but I will remove it from my patch if it is
deemed worthy of back-patching in your other thread.
Regards,
--
Ronan Dunklaucommit 91210952cf435995c9db6b539bf29f6fd744be70
Author: Ronan Dunklau
Date: Tue Jul 6 16:34:31 2021 +0200
Use optimized single-datum tuplesort in ExecSort
Previously, with optimization was only done in ExecAgg.
Doing this optimization on regular sort nodes provides for a nice
performance improvement, when the input / output tuple has only one
attribute.
diff --git a/src/backend/executor/nodeSort.c b/src/backend/executor/nodeSort.c
index b99027e0d7..df4e79c6ba 100644
--- a/src/backend/executor/nodeSort.c
+++ b/src/backend/executor/nodeSort.c
@@ -29,6 +29,10 @@
* which saves the results in a temporary file or memory. After the
* initial call, returns a tuple from the file with each call.
*
+ * The tuplesort can either occur on the whole tuple (this is the nominal
+ * case) or, when the input / output tuple consists of only one attribute,
+ * we switch to the tuplesort_*_datum API, optimized for that specific case.
+ *
* Conditions:
* -- none.
*
@@ -86,31 +90,64 @@ ExecSort(PlanState *pstate)
outerNode = outerPlanState(node);
tupDesc = ExecGetResultType(outerNode);
- tuplesortstate = tuplesort_begin_heap(tupDesc,
- plannode->numCols,
- plannode->sortColIdx,
- plannode->sortOperators,
- plannode->collations,
- plannode->nullsFirst,
- work_mem,
- NULL,
- node->randomAccess);
+ /*
+ * Switch to the tuplesort_*_datum interface when we have only one
+ * key, as it is much more efficient especially when the type is
+ * pass-by-value.
+ */
+ if (tupDesc->natts == 1)
+ {
+ node->datumSort = true;
+ tuplesortstate = tuplesort_begin_datum(TupleDescAttr(tupDesc, 0)->atttypid,
+ plannode->sortOperators[0],
+ plannode->collations[0],
+ plannode->nullsFirst[0],
+ work_mem,
+ NULL,
+ node->randomAccess);
+ }
+ else
+ tuplesortstate = tuplesort_begin_heap(tupDesc,
+ plannode->numCols,
+ plannode->sortColIdx,
+ plannode->sortOperators,
+ plannode->collations,
+ plannode->nullsFirst,
+ work_mem,
+ NULL,
+ node->randomAccess);
if (node->bounded)
tuplesort_set_bound(tuplesortstate, node->bound);
node->tuplesortstate = (void *) tuplesortstate;
/*
- * Scan the subplan and feed all the tuples to tuplesort.
+ * Scan the subplan and feed all the tuples to tuplesort, using either
+ * the _putdatum or _puttupleslot API as appropriate.
*/
-
- for (;;)
+ if (node->datumSort)
{
- slot = ExecProcNode(outerNode);
-
- if (TupIsNull(slot))
-break;
-
- tuplesort_puttupleslot(tuplesortstate, slot);
+ for (;;)
+ {
+slot = ExecProcNode(outerNode);
+
+if (TupIsNull(slot))
+ break;
+slot_getsomeattrs(slot, 1);
+tuplesort_putdatum(tuplesortstate,
+ slot->tts_values[0],
+ slot->tts_isnull[0]);
+ }
+ }
+ else
+ {
+ for (;;)
+ {
+slot = ExecProcNode(outerNode);
+
+if (TupIsNull(slot))
+ break;
+tuplesort_puttupleslot(tup
Re: ATTACH PARTITION locking documentation for DEFAULT partitions
On Mon, 12 Jul 2021 at 15:28, David Rowley wrote:
>
> On Tue, 13 Jul 2021 at 00:14, Matthias van de Meent
> wrote:
> > Sorry for the delay. I think that covers the basics of what I was
> > missing in these docs, and although it does not cover the recursive
> > 'if the check is implied by constraints don't lock this partition',
> > I'd say that your suggested patch is good enough. Thanks for looking
> > over this.
>
> Isn't that covered the following?
>
> +
> + Further locks must also be held on all sub-partitions if the table
> being
> + attached is itself a partitioned table. Likewise if the default
> + partition is itself a partitioned table. The locking of the
> + sub-partitions can be avoided by adding a CHECK
> + constraint as described in
> + .
>
The exact behaviour is (c.q. QueuePartitionConstraintValidation in
tablecmds.c:17072), for each partition of this table:
1.) if the existing constraints imply the new constraints: return to .
2.) lock this partition with ACCESS EXCLUSIVE
3.) if this is a partitioned table, for each direct child partition,
execute this algorithm.
The algoritm as described in your patch implies that this recursive
locking is conditional on _only_ the check-constraints of the topmost
partition ("performed whilst holding ... and all of its
sub-partitions, if any"), whereas actually the locking on each
(sub-)partition is determined by the constraints of the hierarchy down
to that child partition. It in actuality, this should not matter much,
but this is a meaningful distinction that I wanted to call out.
Regardless of the distinction between actual locking behaviour and
this documentation, we might not want to document this specific
algorithm, as this algorithm might be changed in future versions, and
the proposed documentation leaves a little wiggleroom in changing the
locking behaviour without needing to update the docs.
Kind regards,
Matthias van de Meent
Re: Column Filtering in Logical Replication
On 2021-Jul-12, Tomas Vondra wrote: > FWIW "make check" fails for me with this version, due to segfault in > OpenTableLists. Apparenly there's some confusion - the code expects the > list to contain PublicationTable nodes, and tries to extract the > RangeVar from the elements. But the list actually contains RangeVar, so > this crashes and burns. See the attached backtrace. > > I'd bet this is because the patch uses list of RangeVar in some cases > and list of PublicationTable in some cases, similarly to the "row > filtering" patch nearby. IMHO this is just confusing and we should > always pass list of PublicationTable nodes. +1 don't make the code guess what type of list it is. Changing all the uses of that node to deal with PublicationTable seems best. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Cuando no hay humildad las personas se degradan" (A. Christie)
Re: Introduce pg_receivewal gzip compression tests
‐‐‐ Original Message ‐‐‐ On Monday, July 12th, 2021 at 13:00, Gilles Darold wrote: > Le 12/07/2021 à 12:27, gkokola...@pm.me a écrit : > > > > > > Shouldn't this be coded as a loop going through @gzip_wals? > > > > > > > > > > I would hope that there is only one gz file created. There is a line > > > > > > > > further up that tests exactly that. > > > > > > > > - is (scalar(@gzip_wals), 1, "one gzip compressed WAL was created"); > > > > > > > > Let me amend that. The line should be instead: > > > > > > is (scalar(keys @gzip_wals), 1, "one gzip compressed WAL was created"); > > > > > > To properly test that there is one entry. > > > > > > Let me provide with v2 to fix this. > > The following tests are not correct in Perl even if Perl returns the > > right value. > > + is (scalar(keys @gzip_wals), 1, "one gzip compressed WAL was created"); > > + is (scalar(keys @gzip_partial_wals), 1, > > + "one partial gzip compressed WAL was created"); > > Function keys or values are used only with hashes but here you are using > > arrays. To obtain the length of the array you can just use the scalar > > function as Perl returns the length of the array when it is called in a > > scalar context. Please use the following instead: > > + is (scalar(@gzip_wals), 1, "one gzip compressed WAL was created"); > > + is (scalar(@gzip_partial_wals), 1, > > + "one partial gzip compressed WAL was created"); You are absolutely correct. I had used that in v1, yet since it got called out I doubted myself, assumed I was wrong and the rest is history. I shall ament the amendment for v3 of the patch. Cheers, //Georgios > > > - > > Gilles Darold > > http://www.darold.net/
Re: Introduce pg_receivewal gzip compression tests
‐‐‐ Original Message ‐‐‐
On Monday, July 12th, 2021 at 13:04, Michael Paquier
wrote:
> On Mon, Jul 12, 2021 at 09:42:32AM +, gkokola...@pm.me wrote:
>
> > This to my understanding means that gzip is expected to exist.
> >
> > If this is correct, then simply checking for the headers should
> >
> > suffice, since that is the only dependency for the files to be
> >
> > created.
>
> You cannot expect this to work on Windows when it comes to MSVC for
>
> example, as gzip may not be in the environment PATH so the test would
>
> fail hard. Let's just rely on $ENV{GZIP} instead, and skip the test
>
> if it is not defined.
I am admittedly not so well versed on Windows systems. Thank you for
informing me.
Please find attached v3 of the patch where $ENV{GZIP_PROGRAM} is used
instead. To the best of my knowledge one should avoid using $ENV{GZIP}
because that would translate to the obsolete, yet used environment
variable GZIP which holds a set of default options for gzip. In essence
it would be equivalent to executing:
GZIP=gzip gzip --test
which can result to errors similar to:
gzip: gzip: non-option in GZIP environment variable
Cheers,
//Georgios
>
>
> Michael
v3-0001-Introduce-pg_receivewal-gzip-compression-tests.patch
Description: Binary data
Re: Zedstore - compressed in-core columnar storage
Greetings. Thanks for the project. I see the code in github has not been updated for a long time, is it still in active development? Thanks -- Best Regards Andy Fan (https://www.aliyun.com/)
Re: Inaccurate error message when set fdw batch_size to 0
On 2021/07/09 11:41, Bharath Rupireddy wrote:
PSA v6 patch.
Thanks for updating the patch!
+
+ Avoid Using non-negative Word in Error
Messages
+
+
+Do not use non-negative word in error messages as it looks
+ambiguous. Instead, use foo must be an integer value greater than
zero
+or foo must be an integer value greater than or equal to
zero
+if option foo expects an integer value.
+
+
It seems suitable to put this guide under "Tricky Words to Avoid"
rather than adding it as separate section. Thought?
- if (nworkers < 1)
+ if (nworkers <= 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-errmsg("number of workers must be a positive
integer")));
+errmsg("number of workers must be an integer value
greater than zero")));
You replaced "positve" with "greater than zero". So the error message
style guide should mention not only "non-negative" but also "positive"
(probably also "negative") keyword?
If this is true, there are still many messages using "positive" or "negative"
keyword as follows. We should also fix them at all? Of course,
which would increase the change too big unnecessarily, I'm afraid, though..
src/backend/storage/ipc/signalfuncs.c: errmsg("\"timeout\"
must not be negative")));
src/backend/commands/functioncmds.c:
errmsg("COST must be positive")));
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Use WaitLatch for {pre, post}_auth_delay instead of pg_usleep
Hi,
As suggested in [1], starting a new thread for discussing $subject
separately. {pre, post}_auth_delay waiting logic currently uses
pg_usleep which can't detect postmaster death. So, there are chances
that some of the backends still stay in the system even when a
postmaster crashes (for whatever reasons it may be). Please have a
look at the attached patch that does $subject. I pulled out some of
the comments from the other thread related to the $subject, [2], [3],
[4], [5].
[1] - https://www.postgresql.org/message-id/YOv8Yxd5zrbr3k%2BH%40paquier.xyz
[2] - https://www.postgresql.org/message-id/162764.1624892517%40sss.pgh.pa.us
[3] -
https://www.postgresql.org/message-id/20210705.145251.462698229911576780.horikyota.ntt%40gmail.com
[4] -
https://www.postgresql.org/message-id/flat/2021070513.GD20766%40tamriel.snowman.net
[5] - https://www.postgresql.org/message-id/YOOnlP4NtWVzfsyb%40paquier.xyz
Regards,
Bharath Rupireddy.
v1-0001-Use-a-WaitLatch-for-Pre-and-Post-Auth-Delay.patch
Description: Binary data
Re: Is tuplesort meant to support bounded datum sorts?
David Rowley writes: > It looks like this has likely never come up before because the only > time we use tuplesort_set_bound() is in nodeSort.c and > nodeIncrementalSort.c, none of those currently use datum sorts. > However, I'm thinking this is still a bug that should be fixed > separately from Ronan's main patch. Yeah, I think you're right. The comment seems a little confused though. Maybe there's no need for it at all --- there's equivalent code in e.g. writetup_datum that has no comment. regards, tom lane
Re: proposal - psql - use pager for \watch command
On Mon, Jul 12, 2021 at 4:29 AM Thomas Munro wrote: > > On Sun, Jul 11, 2021 at 1:18 AM vignesh C wrote: > > On Wed, May 12, 2021 at 5:45 PM Pavel Stehule > > wrote: > > > looks so with your patch psql doesn't work on ms > > Here's a fix for Windows. The pqsignal() calls are #ifdef'd out. I > also removed a few lines that were added after commit 3a513067 but > aren't needed anymore after fae65629. Thanks for fixing the comments, CFbot also passes for the same. I have changed the status back to "Ready for Committer". Regards, Vignesh
Re: Protect against possible memory corruption (src/backend/access/nbtree/nbtxlog.c)
Em seg., 12 de jul. de 2021 às 05:20, Heikki Linnakangas
escreveu:
> On 12/07/2021 02:34, Ranier Vilela wrote:
> > If it is not possible, know the upper limits, before the loop.
> > It is necessary to do this inside the loop.
>
> > @@ -49,10 +47,14 @@ _bt_restore_page(Page page, char *from, int len)
> >* To get the items back in the original order, we add them to the
> page in
> >* reverse. To figure out where one tuple ends and another
> begins, we
> >* have to scan them in forward order first.
> > + * Check the array upper limit to not overtake him.
> >*/
> > i = 0;
> > - while (from < end)
> > + while (from < end && i <= MaxIndexTuplesPerPage)
> > {
> > + IndexTupleData itupdata;
> > + Sizeitemsz;
> > +
> > /*
> >* As we step through the items, 'from' won't always be
> properly
> >* aligned, so we need to use memcpy(). Further, we use
> Item (which
>
> If we bother checking it, we should throw an error if the check fails,
> not just silently soldier on. Also, shouldn't it be '<', not '<='?
Should be '<', you are right.
In
> general though, we don't do much checking on WAL records, we assume that
> the contents are sane. It would be nice to add more checks and make WAL
> redo routines more robust to corrupt records, but this seems like an odd
> place to start.
>
If WAL records can't be corrupted at _bt_restore_page, that's ok, it's safe.
> I committed the removal of bogus assignment to 'from'. Thanks!
>
Thanks for the commit.
regards,
Ranier Vilela
Re: PROXY protocol support
On Fri, Jul 9, 2021 at 1:42 AM Jacob Champion wrote:
>
> Hi Magnus,
>
> I'm only just starting to page this back into my head, so this is by no
> means a full review of the v7 changes -- just stuff I've noticed over
> the last day or so of poking around.
>
> On Tue, 2021-06-29 at 11:48 +0200, Magnus Hagander wrote:
> > On Thu, Mar 11, 2021 at 12:05 AM Jacob Champion
> > wrote:
> > > On Tue, 2021-03-09 at 11:25 +0100, Magnus Hagander wrote:
> > > > - \x0 : LOCAL : [...] The receiver must accept this connection as
> > > > valid and must use the real connection endpoints and discard the
> > > > protocol block including the family which is ignored.
> > >
> > > So we should ignore the entire "protocol block" (by which I believe
> > > they mean the protocol-and-address-family byte) in the case of LOCAL,
> > > and just accept it with the original address info intact. That seems to
> > > match the sample code in the back of the spec. The current behavior in
> > > the patch will apply the PROXY behavior incorrectly if the sender sends
> > > a LOCAL header with something other than UNSPEC -- which is strange
> > > behavior but not explicitly prohibited as far as I can see.
> >
> > Yeah, I think we do the right thing in the "right usecase".
>
> The current implementation is, I think, stricter than the spec asks
> for. We're supposed to ignore the family for LOCAL cases, and it's not
> clear to me whether we're supposed to also ignore the entire "fam"
> family-and-protocol byte (the phrase "protocol block" is not actually
> defined in the spec).
>
> It's probably not a big deal in practice, but it could mess with
> interoperability for lazier proxy implementations. I think I'll ask the
> HAProxy folks for some clarification tomorrow.
Thanks!
Yeah, I have no problem being stricter than necessary, unless that
actually causes any interop problems. It's a lot worse to not be
strict enough..
> > + proxy_servers (string)
> > +
> > + proxy_servers configuration
> > parameter
> > +
> > +
> > +
> > +
> > +A comma separated list of one or more host names, cidr
> > specifications or the
> > +literal unix, indicating which proxy servers to
> > trust when
> > +connecting on the port specified in > />.
>
> The documentation mentions that host names are valid in proxy_servers,
> but check_proxy_servers() uses the AI_NUMERICHOST hint with
> getaddrinfo(), so host names get rejected.
Ah, good point. Should say "ip addresses".
>
> > + GUC_check_errdetail("Invalid IP addrress %s", tok);
>
> s/addrress/address/
Oops.
> I've been thinking more about your earlier comment:
>
> > An interesting thing is what to do about
> > inet_server_addr/inet_server_port. That sort of loops back up to the
> > original question of where/how to expose the information about the
> > proxy in general (since right now it just logs). Right now you can
> > actually use inet_server_port() to see if the connection was proxied
> > (as long as it was over tcp).
>
> IMO these should return the "virtual" dst_addr/port, instead of
> exposing the physical connection information to the client. That way,
> if you intend for your proxy to be transparent, you're not advertising
> your network internals to connected clients. It also means that clients
> can reasonably expect to be able to reconnect to the addr:port that we
> give them, and prevents confusion if the proxy is using an address
> family that the client doesn't even support (e.g. the client is IPv4-
> only but the proxy connects via IPv6).
That reasoning I think makes a lot of sense, especially with the
comment about being able to connect back to it.
The question at that point extends to, would we also add extra
functions to get the data on the proxy connection itself? Maybe add a
inet_proxy_addr()/inet_proxy_port()? Better names?
PFA a patch that fixes the above errors, and changes
inet_server_addr()/inet_server_port(). Does not yet add anything to
receive the actual local port in this case.
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 02f0489112..a3ff09b3ac 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -353,6 +353,15 @@ hostnogssenc database user
+
+ If is enabled and the
+ connection is made through a proxy server using the PROXY
+ protocol, the actual IP address of the client will be used
+ for matching. If a connection is made through a proxy server
+ not using the PROXY protocol, the IP address of the
+ proxy server will be used.
+
+
These fields do not apply to local records.
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 381d8636ab..778a20a179 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -682,6 +682,56 @@ include_dir 'conf.d'
Re: Enhanced error message to include hint messages for redundant options error
On Sun, Jul 11, 2021 at 3:23 PM Dean Rasheed wrote:
>
> On Sat, 10 Jul 2021 at 18:09, vignesh C wrote:
> >
> > I'm planning to handle conflicting errors separately after this
> > current work is done, once the patch is changed to have just the valid
> > scenarios(removing the scenarios you have pointed out) existing
> > function can work as is without any changes. Thoughts?
>
> Ah OK, that might be reasonable. Perhaps, then errorDuplicateDefElem()
> and errorConflictingDefElem() would be better than what I originally
> suggested.
>
> BTW, another case I spotted was this:
>
> copy (select 1) to stdout csv csv header;
> ERROR: option "format" specified more than once
> LINE 1: copy (select 1) to stdout csv csv header;
> ^
>
Thanks for your comments, I have made the changes for the same in the
V10 patch attached.
Thoughts?
Regards,
Vignesh
From c882bd3f8ebc7a8a21b8481234286aa8e2c9aee1 Mon Sep 17 00:00:00 2001
From: vignesh
Date: Mon, 12 Jul 2021 09:12:34 +0530
Subject: [PATCH v10] Enhance error message to include option name in case of
duplicate option error.
Enhanced error message to include option name in case of duplication
option error, so that the user can easily identify the error.
---
contrib/file_fdw/file_fdw.c | 10 +-
src/backend/catalog/aclchk.c| 11 +-
src/backend/commands/copy.c | 54 ++
src/backend/commands/dbcommands.c | 70 +++-
src/backend/commands/define.c | 20
src/backend/commands/extension.c| 20 +---
src/backend/commands/foreigncmds.c | 18 ++--
src/backend/commands/functioncmds.c | 53 +++--
src/backend/commands/publicationcmds.c | 27 +++--
src/backend/commands/sequence.c | 45 ++--
src/backend/commands/subscriptioncmds.c | 62 +--
src/backend/commands/tablecmds.c| 4 +-
src/backend/commands/typecmds.c | 31 ++
src/backend/commands/user.c | 112 +---
src/backend/parser/parse_utilcmd.c | 4 +-
src/backend/replication/pgoutput/pgoutput.c | 20 +---
src/backend/tcop/utility.c | 20 ++--
src/include/commands/defrem.h | 8 +-
src/include/commands/publicationcmds.h | 4 +-
src/include/commands/subscriptioncmds.h | 4 +-
src/include/commands/typecmds.h | 2 +-
src/include/commands/user.h | 2 +-
src/test/regress/expected/copy2.out | 24 +++--
src/test/regress/expected/foreign_data.out | 8 +-
src/test/regress/expected/publication.out | 4 +-
25 files changed, 209 insertions(+), 428 deletions(-)
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 2c2f149fb0..eb7edb695b 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -290,10 +290,7 @@ file_fdw_validator(PG_FUNCTION_ARGS)
else if (strcmp(def->defname, "force_not_null") == 0)
{
if (force_not_null)
-ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("conflicting or redundant options"),
- errhint("Option \"force_not_null\" supplied more than once for a column.")));
+errorDuplicateDefElem(def, NULL);
force_not_null = def;
/* Don't care what the value is, as long as it's a legal boolean */
(void) defGetBoolean(def);
@@ -302,10 +299,7 @@ file_fdw_validator(PG_FUNCTION_ARGS)
else if (strcmp(def->defname, "force_null") == 0)
{
if (force_null)
-ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("conflicting or redundant options"),
- errhint("Option \"force_null\" supplied more than once for a column.")));
+errorDuplicateDefElem(def, NULL);
force_null = def;
(void) defGetBoolean(def);
}
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 53392414f1..c5b9f3cf70 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -59,6 +59,7 @@
#include "catalog/pg_ts_template.h"
#include "catalog/pg_type.h"
#include "commands/dbcommands.h"
+#include "commands/defrem.h"
#include "commands/event_trigger.h"
#include "commands/extension.h"
#include "commands/proclang.h"
@@ -921,19 +922,13 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s
if (strcmp(defel->defname, "schemas") == 0)
{
if (dnspnames)
-ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("conflicting or redundant options"),
- parser_errposition(pstate, defel->location)));
+errorDuplicateDefElem(defel, pstate);
dnspnames = defel;
}
else if (strcmp(defel->defname, "roles") == 0)
{
if (drolespecs)
-ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("conflicting or redundant options"),
- parser_errposition(pstate, defel->location)));
+errorDuplicateDefElem(defel, pstate);
drolespecs = defel;
Re: Inaccurate error message when set fdw batch_size to 0
On Mon, Jul 12, 2021 at 9:20 PM Fujii Masao wrote:
> +
> + Avoid Using non-negative Word in Error
> Messages
> +
> +
> +Do not use non-negative word in error messages as it looks
> +ambiguous. Instead, use foo must be an integer value greater than
> zero
> +or foo must be an integer value greater than or equal to
> zero
> +if option foo expects an integer value.
> +
> +
>
> It seems suitable to put this guide under "Tricky Words to Avoid"
> rather than adding it as separate section. Thought?
+1. I will change.
> - if (nworkers < 1)
> + if (nworkers <= 0)
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> -errmsg("number of workers must be a positive
> integer")));
> +errmsg("number of workers must be an integer
> value greater than zero")));
>
> You replaced "positve" with "greater than zero". So the error message
> style guide should mention not only "non-negative" but also "positive"
> (probably also "negative") keyword?
The main focus of the patch is to replace the ambiguous "non-negative"
work in the error message. Let's keep it to that. However, I changed
below two messages too to keep them in sync with nearby messages.
Also, there seems to be an ambiguity in treating 0 as a positive or
negative integer, I thought it makes sense to replace them. But, if
others don't agree, I'm happy to revert.
- errmsg("modulus for hash partition must be a positive integer")));
+ errmsg("modulus for hash partition must be an integer value greater
than zero")));
- errmsg("number of workers must be a positive integer")));
+ errmsg("number of workers must be an integer value greater than zero")));
> If this is true, there are still many messages using "positive" or "negative"
> keyword as follows. We should also fix them at all? Of course,
> which would increase the change too big unnecessarily, I'm afraid, though..
>
> src/backend/storage/ipc/signalfuncs.c:
> errmsg("\"timeout\" must not be negative")));
> src/backend/commands/functioncmds.c:
> errmsg("COST must be positive")));
You are right. The change is going to be an unnecessary one. So, let's
not do that.
Regards,
Bharath Rupireddy.
Re: Introduce pg_receivewal gzip compression tests
‐‐‐ Original Message ‐‐‐
On Monday, July 12th, 2021 at 17:07, wrote:
> ‐‐‐ Original Message ‐‐‐
>
> On Monday, July 12th, 2021 at 13:04, Michael Paquier mich...@paquier.xyz
> wrote:
>
> > On Mon, Jul 12, 2021 at 09:42:32AM +, gkokola...@pm.me wrote:
> >
> > > This to my understanding means that gzip is expected to exist.
> > >
> > > If this is correct, then simply checking for the headers should
> > >
> > > suffice, since that is the only dependency for the files to be
> > >
> > > created.
> >
> > You cannot expect this to work on Windows when it comes to MSVC for
> >
> > example, as gzip may not be in the environment PATH so the test would
> >
> > fail hard. Let's just rely on $ENV{GZIP} instead, and skip the test
> >
> > if it is not defined.
>
> I am admittedly not so well versed on Windows systems. Thank you for
>
> informing me.
>
> Please find attached v3 of the patch where $ENV{GZIP_PROGRAM} is used
>
> instead. To the best of my knowledge one should avoid using $ENV{GZIP}
>
> because that would translate to the obsolete, yet used environment
>
> variable GZIP which holds a set of default options for gzip. In essence
>
> it would be equivalent to executing:
>
> GZIP=gzip gzip --test
>
> which can result to errors similar to:
>
> gzip: gzip: non-option in GZIP environment variable
>
After a bit more thinking, I went ahead and added on top of v3 a test
verifying that the gzip program can actually be called.
Please find v4 attached.
Cheers,
//Georgios
>
> > Michael
v4-0001-Introduce-pg_receivewal-gzip-compression-tests.patch
Description: Binary data
printf %s with NULL pointer (was Re: BUG #17098: Assert failed on composing an error message when adding a type to an extension being dropped)
[ moved from -bugs list for more visibility ]
I wrote:
> However, that root issue is converted from a relatively minor bug into
> a server crash because snprintf.c treats a NULL pointer passed to %s
> as a crash-worthy error. I have advocated for that behavior in the
> past, but I'm starting to wonder if it wouldn't be wiser to change
> over to the glibc-ish behavior of printing "(null)" or the like.
> It seems like we've long since found all the interesting bugs that
> the assert-or-crash behavior could reveal, and now we're down to
> weird corner cases where its main effect is to weaken our robustness.
I did a little more thinking about this. I believe the strongest
argument for having snprintf.c crash on NULL is that it keeps us
from relying on having more-forgiving behavior in case we're using
platform-supplied *printf functions (cf commit 0c62356cc). However,
that is only relevant for code that's meant to go into pre-v12 branches,
since we stopped using libc's versions of these functions in v12.
So one plausible way to approach this is to say that we should wait
until v11 is EOL and then change it.
However, that feels overly conservative to me. I doubt that anyone
is *intentionally* relying on *printf not crashing on a NULL pointer.
For example, in the case that started this thread:
if (OidIsValid(oldExtension))
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("%s is already a member of extension \"%s\"",
getObjectDescription(&object, false),
get_extension_name(oldExtension;
the problem is failure to consider the possibility that
get_extension_name could return NULL due to a just-committed
concurrent DROP EXTENSION. I'm afraid there are a lot of
corner cases like that still lurking.
So my feeling about this is that switching snprintf.c's behavior
would produce some net gain in robustness for v12 and up, while
not making things any worse for the older branches. I still hold
to the opinion that we've already flushed out all the cases of
passing NULL that we're likely to find via ordinary testing.
Thoughts?
regards, tom lane
Re: "debug_invalidate_system_caches_always" is too long
Noah Misch writes: > On Thu, Jul 08, 2021 at 04:34:55PM -0400, Tom Lane wrote: >> Robert Haas writes: >>> I like debug_discard_caches best. >> I can live with that. Anyone strongly against it? > I like it. Hearing no votes against, here's a proposed patch for that. (This is for HEAD; I expect v14 will need additional adjustments in release-14.sgml) regards, tom lane diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index b8561d6a3c..e5bbf3b0af 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -9340,11 +9340,11 @@ WARNING: there is no transaction in progress -- Change application_name of remote connection to special one -- so that we can easily terminate the connection later. ALTER SERVER loopback OPTIONS (application_name 'fdw_retry_check'); --- If debug_invalidate_system_caches_always is active, it results in +-- If debug_discard_caches is active, it results in -- dropping remote connections after every transaction, making it -- impossible to test termination meaningfully. So turn that off -- for this test. -SET debug_invalidate_system_caches_always = 0; +SET debug_discard_caches = 0; -- Make sure we have a remote connection. SELECT 1 FROM ft1 LIMIT 1; ?column? @@ -9386,7 +9386,7 @@ SELECT 1 FROM ft1 LIMIT 1;-- should fail ERROR: 08006 \set VERBOSITY default COMMIT; -RESET debug_invalidate_system_caches_always; +RESET debug_discard_caches; -- = -- test connection invalidation cases and postgres_fdw_get_connections function -- = diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index c283e74715..fe503ed6c3 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -2831,11 +2831,11 @@ ROLLBACK; -- so that we can easily terminate the connection later. ALTER SERVER loopback OPTIONS (application_name 'fdw_retry_check'); --- If debug_invalidate_system_caches_always is active, it results in +-- If debug_discard_caches is active, it results in -- dropping remote connections after every transaction, making it -- impossible to test termination meaningfully. So turn that off -- for this test. -SET debug_invalidate_system_caches_always = 0; +SET debug_discard_caches = 0; -- Make sure we have a remote connection. SELECT 1 FROM ft1 LIMIT 1; @@ -2861,7 +2861,7 @@ SELECT 1 FROM ft1 LIMIT 1;-- should fail \set VERBOSITY default COMMIT; -RESET debug_invalidate_system_caches_always; +RESET debug_discard_caches; -- = -- test connection invalidation cases and postgres_fdw_get_connections function diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 381d8636ab..d1b889b80f 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -10341,10 +10341,10 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' - - debug_invalidate_system_caches_always (integer) + + debug_discard_caches (integer) - debug_invalidate_system_caches_always configuration parameter + debug_discard_caches configuration parameter @@ -10369,7 +10369,7 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' This parameter is supported when -CLOBBER_CACHE_ENABLED was defined at compile time +DISCARD_CACHES_ENABLED was defined at compile time (which happens automatically when using the configure option --enable-cassert). In production builds, its value diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml index 3077530c7b..e62742850a 100644 --- a/doc/src/sgml/ref/initdb.sgml +++ b/doc/src/sgml/ref/initdb.sgml @@ -388,17 +388,6 @@ PostgreSQL documentation Other, less commonly used, options are also available: - - --clobber-cache - - -Run the bootstrap backend with the -debug_invalidate_system_caches_always=1 option. -This takes a very long time and is only of use for deep debugging. - - - - -d --debug @@ -413,6 +402,17 @@ PostgreSQL documentation + + --discard-caches + + +Run the bootstrap backend with the +debug_discard_caches=1 option. +This takes a very long time and is only of use for deep debugging. + + + + -L directory diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml index c35e036028..acc7a50c2f 100644 --- a/doc/src/sgml/regress.sgml +++ b/doc/src/sgml/regress.sgml @@ -373,7 +373,7 @@ make che
Re: Remove useless int64 range checks on BIGINT sequence MINVALUE/MAXVALUE values
On 12.07.21 10:44, David Rowley wrote: What I was trying to get to here was something that was more reasonable that might make sense to commit. I'm just not certain where Peter stands on this now that the latest patch is a net zero when it comes to adding lines. Peter? Your version looks better to me than the original version, but I'm still -0.05 on changing this at all.
Re: Partition Check not updated when insert into a partition
On 2021-Jun-23, houzj.f...@fujitsu.com wrote:
> For a multi-level partition, for example: table 'A' is partition of table
> 'B', and 'B' is also partition of table 'C'. After I 'ALTER TABLE C DETACH B',
> I thought partition constraint check of table 'C' does not matter anymore if
> INSERT INTO table 'A'. But it looks like the relcache of 'A' is not
> invalidated
> after detaching 'B'. And the relcache::rd_partcheck still include the
> partition
> constraint of table 'C'. Note If I invalidate the table 'A''s relcache
> manually,
> then next time the relcache::rd_partcheck will be updated to the expected
> one which does not include partition constraint check of table 'C'.
Hmm, if I understand correctly, this means that we need to invalidate
relcache for all partitions of the partition being detached. Maybe like
in the attached WIP ("XXX VERY CRUDE XXX DANGER EATS DATA") patch, which
solves what you complained about, but I didn't run any other tests.
(Also, in the concurrent case I think this should be done during the
first transaction, so this patch is wrong for it.)
Did you have a misbehaving test for the ATTACH case?
--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
"I dream about dreams about dreams", sang the nightingale
under the pale moon (Sandman)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 03dfd2e7fa..b24fa05e42 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -18107,6 +18107,20 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
* included in its partition descriptor.
*/
CacheInvalidateRelcache(rel);
+
+ /*
+ * If the partition is partitioned, invalidate relcache for all its
+ * partitions also, because ... XXX explain
+ */
+ if (partRel->rd_rel->relispartition)
+ {
+ PartitionDesc partdesc = RelationGetPartitionDesc(partRel, true);
+
+ for (int i = 0; i < partdesc->nparts; i++)
+ {
+ CacheInvalidateRelcacheByRelid(partdesc->oids[i]);
+ }
+ }
}
/*
Re: [PATCH v3 1/1] Fix detection of preadv/pwritev support for OSX.
On 06.07.21 22:34, Tom Lane wrote: 2. We'd really like to use preadv/pwritev where available. A couple of things that I haven't seen made clear in this thread yet: - Where is the availability boundary for preadv/pwritev on macOS? - What is the impact of having vs. not having these functions? I maintain that MACOSX_DEPLOYMENT_TARGET is not only not the right approach to that, but it's actually counterproductive. It forces you to build for the lowest common denominator, ie the oldest macOS release you want to support. Even when running on a release that has pwritev, your build will never use it. I think this is just the way that building backward-compatible binaries on macOS (and Windows) works. You have to pick a target that is old enough to capture enough of your audience but not too old to miss out on interesting new OS features. People who build GUI applications for macOS, iOS, etc. face this trade-off all the time; for POSIX-level programming things just move slower so that the questions present themselves less often. I don't think we need to go out of our way to fight this system. This is something users will have opted into after all. Users who want Linux-style rebuilt-for-every-release binaries have those options available on macOS as well.
Re: speed up verifying UTF-8
I wrote: > I don't think the new structuring will pose any challenges for rebasing 0002, either. This might need some experimentation, though: > > + * Subroutine of pg_utf8_verifystr() to check on char. Returns the length of the > + * character at *s in bytes, or 0 on invalid input or premature end of input. > + * > + * XXX: could this be combined with pg_utf8_verifychar above? > + */ > +static inline int > +pg_utf8_verify_one(const unsigned char *s, int len) > > It seems like it would be easy to have pg_utf8_verify_one in my proposed pg_utf8.h header and replace the body of pg_utf8_verifychar with it. 0001: I went ahead and tried this for v15, and also attempted some clean-up: - Rename pg_utf8_verify_one to pg_utf8_verifychar_internal. - Have pg_utf8_verifychar_internal return -1 for invalid input to match other functions in the file. We could also do this for check_ascii, but it's not quite the same thing, because the string could still have valid bytes in it, just not enough to advance the pointer by the stride length. - Remove hard-coded numbers (not wedded to this). - Use a call to pg_utf8_verifychar in the slow path. - Reduce pg_utf8_verifychar to thin wrapper around pg_utf8_verifychar_internal. The last two aren't strictly necessary, but it prevents bloating the binary in the slow path, and aids readability. For 0002, this required putting pg_utf8_verifychar* in src/port. (While writing this I noticed I neglected to explain that with a comment, though) Feedback welcome on any of the above. Since by now it hardly resembles the simdjson (or Fuchsia for that matter) fallback that it took inspiration from, I've removed that mention from the commit message. 0002: Just a rebase to work with the above. One possible review point: We don't really need to have separate control over whether to use special instructions for CRC and UTF-8. It should probably be just one configure knob, but having them separate is perhaps easier to review. -- John Naylor EDB: http://www.enterprisedb.com v15-0001-Rewrite-pg_utf8_verifystr-for-speed.patch Description: Binary data v15-0002-Use-SSE-instructions-for-pg_utf8_verifystr-where.patch Description: Binary data
Re: row filtering for logical replication
On Mon, Jul 12, 2021, at 8:44 AM, Tomas Vondra wrote: > While looking at the other logrep patch [1] (column filtering) I noticed > Alvaro's comment regarding a new parsenode (PublicationTable) not having > read/out/equal/copy funcs. I'd bet the same thing applies here, so > perhaps see if the patch needs the same fix. Good catch! I completely forgot about _copyPublicationTable() and _equalPublicationTable(). -- Euler Taveira EDB https://www.enterprisedb.com/
Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?
On 22.04.21 11:23, Julien Rouhaud wrote: The statistics gathered by the module are made available via a view named pg_stat_statements. This view - contains one row for each distinct database ID, user ID and query - ID (up to the maximum number of distinct statements that the module + contains one row for each distinct database ID, user ID, query ID and + toplevel (up to the maximum number of distinct statements that the module can track). The columns of the view are shown in . I'm having trouble parsing this new sentence. It now says essentially "This view contains one row for each distinct database ID, each distinct user ID, each distinct query ID, and each distinct toplevel." That last part doesn't make sense.
Re: proposal - psql - use pager for \watch command
po 12. 7. 2021 v 18:12 odesílatel vignesh C napsal: > On Mon, Jul 12, 2021 at 4:29 AM Thomas Munro > wrote: > > > > On Sun, Jul 11, 2021 at 1:18 AM vignesh C wrote: > > > On Wed, May 12, 2021 at 5:45 PM Pavel Stehule > wrote: > > > > looks so with your patch psql doesn't work on ms > > > > Here's a fix for Windows. The pqsignal() calls are #ifdef'd out. I > > also removed a few lines that were added after commit 3a513067 but > > aren't needed anymore after fae65629. > > Thanks for fixing the comments, CFbot also passes for the same. I have > changed the status back to "Ready for Committer". > I tested this version with the last release and with a developing version of pspg, and it works very well. Regards Pavel > Regards, > Vignesh >
Re: [PATCH] Don't block HOT update by BRIN index
On 6/30/21 1:43 AM, Josef Šimánek wrote: > st 30. 6. 2021 v 1:20 odesílatel Tomas Vondra > napsal: >> >> >> >> On 6/30/21 12:53 AM, Josef Šimánek wrote: >>> st 30. 6. 2021 v 0:31 odesílatel Josef Šimánek >>> napsal: Hello! Tomáš Vondra has shared a few ideas to improve BRIN index in czech PostgreSQL mail list some time ago [1 , in czech only]. This is first try to implement one of those ideas. Currently BRIN index blocks HOT update even it is not linked tuples directly. I'm attaching the initial patch allowing HOT update even on BRIN indexed columns. This patch went through an initial review on czech PostgreSQL mail list [1]. >>> >>> I just found out current patch is breaking partial-index isolation >>> test. I'm looking into this problem. >>> >> >> The problem is in RelationGetIndexAttrBitmap - the existing code first >> walks indnatts, and builds the indexattrs / hotblockingattrs. But then >> it also inspects expressions and the predicate (by pull_varattnos), and >> the patch fails to do that for hotblockingattrs. Which is why it fails >> for partial-index, because that uses an index with a predicate. >> >> So there needs to be something like: >> >> if (indexDesc->rd_indam->amhotblocking) >> pull_varattnos(indexExpressions, 1, &hotblockingattrs); >> >> if (indexDesc->rd_indam->amhotblocking) >> pull_varattnos(indexPredicate, 1, &hotblockingattrs); >> >> This fixes the failure for me. > > Thanks for the hint. I'm attaching a fixed standalone patch. > Thanks, this version seems to be working fine and passes check-world. So I did another round of review, and all I have are some simple comments: 1) naming stuff (this is very subjective, feel free to disagree) I wonder if we should rename 'amhotblocking' to 'amblockshot' which seems more natural to me? Similarly, maybe rename rd_hotblockingattr to rd_hotattr 2) Do we actually need to calculate and store hotblockingattrs separately in RelationGetIndexAttrBitmap? It seems to me it's either NULL (with amhotblocking=false) or equal to indexattrs. So why not to just get rid of hotblockingattr and rd_hotblockingattr, and do something like case INDEX_ATTR_BITMAP_HOT_BLOCKING: return (amhotblocking) ? bms_copy(rel->rd_hotblockingattr) : NULL; I haven't tried, so maybe I'm missing something? 3) The patch should update indexam.sgml with description of the new field, amhotblocking or how it'll end up named. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Fix comments of heap_prune_chain()
On 2021-Jul-12, ikeda...@oss.nttdata.com wrote: > While I’m reading source codes related to vacuum, I found comments which > don’t seem to fit the reality. I think the commit[1] just forgot to fix them. > What do you think? > > [1] > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=dc7420c2c9274a283779ec19718d2d16323640c0 That sounds believable, but can you be more specific? -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: [PATCH] Don't block HOT update by BRIN index
On 2021-Jul-12, Tomas Vondra wrote: > 2) Do we actually need to calculate and store hotblockingattrs > separately in RelationGetIndexAttrBitmap? It seems to me it's either > NULL (with amhotblocking=false) or equal to indexattrs. So why not to > just get rid of hotblockingattr and rd_hotblockingattr, and do something > like > > case INDEX_ATTR_BITMAP_HOT_BLOCKING: > return (amhotblocking) ? bms_copy(rel->rd_hotblockingattr) : NULL; > > I haven't tried, so maybe I'm missing something? ... What? I thought the whole point is that BRIN indexes do not cause the columns to become part of this set, while all other index types do. If you make them both the same, then there's no point. -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/ "La libertad es como el dinero; el que no la sabe emplear la pierde" (Alvarez)
Re: [PATCH] Don't block HOT update by BRIN index
On 7/12/21 10:37 PM, Alvaro Herrera wrote: > On 2021-Jul-12, Tomas Vondra wrote: > >> 2) Do we actually need to calculate and store hotblockingattrs >> separately in RelationGetIndexAttrBitmap? It seems to me it's either >> NULL (with amhotblocking=false) or equal to indexattrs. So why not to >> just get rid of hotblockingattr and rd_hotblockingattr, and do something >> like >> >> case INDEX_ATTR_BITMAP_HOT_BLOCKING: >> return (amhotblocking) ? bms_copy(rel->rd_hotblockingattr) : NULL; >> >> I haven't tried, so maybe I'm missing something? > > ... What? I thought the whole point is that BRIN indexes do not cause > the columns to become part of this set, while all other index types do. > If you make them both the same, then there's no point. > Well, one of us is confused and it might be me ;-) The point is that BRIN is the only index type with amhotblocking=false, so it would return NULL (and thus it does not block HOT). All other indexes AMs have amblocking=true and so should return rd_indexattr (I forgot to change that in the code chunk). regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [PATCH v3 1/1] Fix detection of preadv/pwritev support for OSX.
On Tue, Jul 13, 2021 at 7:39 AM Peter Eisentraut wrote: > On 06.07.21 22:34, Tom Lane wrote: > > 2. We'd really like to use preadv/pwritev where available. > > A couple of things that I haven't seen made clear in this thread yet: > > - Where is the availability boundary for preadv/pwritev on macOS? Big Sur (11) added these. > - What is the impact of having vs. not having these functions? The impact is very low. PG14 only uses pg_pwritev() to fill in new WAL files as a sort of a warm-up exercise, and it'll happy use lseek() + writev() instead. In future proposals they would be used to do general scatter/gather I/O for data files as I showed in another email[1], but that's way off and far from certain, and even then it's just a matter of avoiding an lseek() call on vectored I/O. As for how long Apple will support 10.15, they don't seem to publish a roadmap, but people seem to say the pattern would have security updates ending some time in 2022 (?). I don't know if EDB targets macOS older than Apple supports, but given the very low impact and all these time frames it seems OK to just not use the new syscalls on macOS for a couple more years at least, whatever mechanism is chosen for that. Clearly there is a more general question though, which is "should we buy into Apple's ABI management system or not", and I don't have a strong opinion on that. One thing I do know is that pthread_barrier_XXX changed from option to required in a recentish POSIX update so I expect the question to come up again eventually. [1] https://www.postgresql.org/message-id/CA%2BhUKGK-563RQWQQF4NLajbQk%2B65gYHdb1q%3D7p3Ob0Uvrxoa9g%40mail.gmail.com
Re: [PATCH] Don't block HOT update by BRIN index
po 12. 7. 2021 v 22:31 odesílatel Tomas Vondra napsal: > > On 6/30/21 1:43 AM, Josef Šimánek wrote: > > st 30. 6. 2021 v 1:20 odesílatel Tomas Vondra > > napsal: > >> > >> > >> > >> On 6/30/21 12:53 AM, Josef Šimánek wrote: > >>> st 30. 6. 2021 v 0:31 odesílatel Josef Šimánek > >>> napsal: > > Hello! > > Tomáš Vondra has shared a few ideas to improve BRIN index in czech > PostgreSQL mail list some time ago [1 , in czech only]. This is first > try to implement one of those ideas. > > Currently BRIN index blocks HOT update even it is not linked tuples > directly. I'm attaching the initial patch allowing HOT update even on > BRIN indexed columns. This patch went through an initial review on > czech PostgreSQL mail list [1]. > >>> > >>> I just found out current patch is breaking partial-index isolation > >>> test. I'm looking into this problem. > >>> > >> > >> The problem is in RelationGetIndexAttrBitmap - the existing code first > >> walks indnatts, and builds the indexattrs / hotblockingattrs. But then > >> it also inspects expressions and the predicate (by pull_varattnos), and > >> the patch fails to do that for hotblockingattrs. Which is why it fails > >> for partial-index, because that uses an index with a predicate. > >> > >> So there needs to be something like: > >> > >> if (indexDesc->rd_indam->amhotblocking) > >> pull_varattnos(indexExpressions, 1, &hotblockingattrs); > >> > >> if (indexDesc->rd_indam->amhotblocking) > >> pull_varattnos(indexPredicate, 1, &hotblockingattrs); > >> > >> This fixes the failure for me. > > > > Thanks for the hint. I'm attaching a fixed standalone patch. > > > > Thanks, this version seems to be working fine and passes check-world. So > I did another round of review, and all I have are some simple comments: > > > 1) naming stuff (this is very subjective, feel free to disagree) > > I wonder if we should rename 'amhotblocking' to 'amblockshot' which > seems more natural to me? > > Similarly, maybe rename rd_hotblockingattr to rd_hotattr OK, I wasn't sure about the naming. > > 2) Do we actually need to calculate and store hotblockingattrs > separately in RelationGetIndexAttrBitmap? It seems to me it's either > NULL (with amhotblocking=false) or equal to indexattrs. So why not to > just get rid of hotblockingattr and rd_hotblockingattr, and do something > like > > case INDEX_ATTR_BITMAP_HOT_BLOCKING: > return (amhotblocking) ? bms_copy(rel->rd_hotblockingattr) : NULL; > > I haven't tried, so maybe I'm missing something? relation->rd_indexattr is currently not used (at least I have not found anything) for anything, except looking if other values are already loaded. /* Quick exit if we already computed the result. */ if (relation->rd_indexattr != NULL) I think it could be replaced with boolean to make it clear other values (rd_keyattr, rd_pkattr, rd_idattr, rd_hotblockingattr) are already loaded. > > 3) The patch should update indexam.sgml with description of the new > field, amhotblocking or how it'll end up named. I'll do. > > regards > > -- > Tomas Vondra > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company
Re: [PATCH] Don't block HOT update by BRIN index
On 7/12/21 10:45 PM, Josef Šimánek wrote: > po 12. 7. 2021 v 22:31 odesílatel Tomas Vondra > napsal: >> >> On 6/30/21 1:43 AM, Josef Šimánek wrote: >>> st 30. 6. 2021 v 1:20 odesílatel Tomas Vondra >>> napsal: On 6/30/21 12:53 AM, Josef Šimánek wrote: > st 30. 6. 2021 v 0:31 odesílatel Josef Šimánek > napsal: >> >> Hello! >> >> Tomáš Vondra has shared a few ideas to improve BRIN index in czech >> PostgreSQL mail list some time ago [1 , in czech only]. This is first >> try to implement one of those ideas. >> >> Currently BRIN index blocks HOT update even it is not linked tuples >> directly. I'm attaching the initial patch allowing HOT update even on >> BRIN indexed columns. This patch went through an initial review on >> czech PostgreSQL mail list [1]. > > I just found out current patch is breaking partial-index isolation > test. I'm looking into this problem. > The problem is in RelationGetIndexAttrBitmap - the existing code first walks indnatts, and builds the indexattrs / hotblockingattrs. But then it also inspects expressions and the predicate (by pull_varattnos), and the patch fails to do that for hotblockingattrs. Which is why it fails for partial-index, because that uses an index with a predicate. So there needs to be something like: if (indexDesc->rd_indam->amhotblocking) pull_varattnos(indexExpressions, 1, &hotblockingattrs); if (indexDesc->rd_indam->amhotblocking) pull_varattnos(indexPredicate, 1, &hotblockingattrs); This fixes the failure for me. >>> >>> Thanks for the hint. I'm attaching a fixed standalone patch. >>> >> >> Thanks, this version seems to be working fine and passes check-world. So >> I did another round of review, and all I have are some simple comments: >> >> >> 1) naming stuff (this is very subjective, feel free to disagree) >> >> I wonder if we should rename 'amhotblocking' to 'amblockshot' which >> seems more natural to me? >> >> Similarly, maybe rename rd_hotblockingattr to rd_hotattr > > OK, I wasn't sure about the naming. > TBH I'm not sure either. >> >> 2) Do we actually need to calculate and store hotblockingattrs >> separately in RelationGetIndexAttrBitmap? It seems to me it's either >> NULL (with amhotblocking=false) or equal to indexattrs. So why not to >> just get rid of hotblockingattr and rd_hotblockingattr, and do something >> like >> >> case INDEX_ATTR_BITMAP_HOT_BLOCKING: >> return (amhotblocking) ? bms_copy(rel->rd_hotblockingattr) : NULL; >> >> I haven't tried, so maybe I'm missing something? > > relation->rd_indexattr is currently not used (at least I have not > found anything) for anything, except looking if other values are > already loaded. > > /* Quick exit if we already computed the result. */ > if (relation->rd_indexattr != NULL) > > I think it could be replaced with boolean to make it clear other > values (rd_keyattr, rd_pkattr, rd_idattr, rd_hotblockingattr) are > already loaded. Well, RelationGetIndexAttrBitmap is accessible from extensions, so it might be used by code passing INDEX_ATTR_BITMAP_ALL. Not sure if there's such code, of course. My point is that for amhotblocking=true the bitmaps seem to be exactly the same, so we can calculate it just once (so replacing it with a bool flag would not save us anything). regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [PATCH] Don't block HOT update by BRIN index
On 2021-Jul-12, Tomas Vondra wrote: > Well, one of us is confused and it might be me ;-) :-) > The point is that BRIN is the only index type with amhotblocking=false, > so it would return NULL (and thus it does not block HOT). All other > indexes AMs have amblocking=true and so should return rd_indexattr (I > forgot to change that in the code chunk). But RelationGetIndexAttrBitmap is called for the table that contains the index (and probably contains some other indexes too), not for one specific index. So the bitmap is about the columns involved in *all* indexes of the table ... -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/ "El destino baraja y nosotros jugamos" (A. Schopenhauer)
Re: Fix comments of heap_prune_chain()
On Mon, 12 Jul 2021 at 13:14, wrote: > > Hi, > > While I’m reading source codes related to vacuum, I found comments which > don’t seem to fit the reality. I think the commit[1] just forgot to fix them. > What do you think? Hmm, yes, those are indeed some leftovers. Some comments on the suggested changes: - * caused by HeapTupleSatisfiesVacuum. We just add entries to the arrays in + * caused by heap_prune_satisfies_vacuum. We just add entries to the arrays in I think that HeapTupleSatisfiesVacuumHorizon might be an alternative correct replacement here. -elog(ERROR, "unexpected HeapTupleSatisfiesVacuum result"); +elog(ERROR, "unexpected heap_prune_satisfies_vacuum result"); The type of the value is HTSV_Result; where HTSV stands for HeapTupleSatisfiesVacuum, so if we were to replace this, I'd go for "unexpected result from heap_prune_satisfies_vacuum" as a message instead. Kind regards, Matthias van de Meent
Re: [PATCH] Don't block HOT update by BRIN index
On 7/12/21 10:55 PM, Alvaro Herrera wrote: > On 2021-Jul-12, Tomas Vondra wrote: > >> Well, one of us is confused and it might be me ;-) > > :-) > >> The point is that BRIN is the only index type with amhotblocking=false, >> so it would return NULL (and thus it does not block HOT). All other >> indexes AMs have amblocking=true and so should return rd_indexattr (I >> forgot to change that in the code chunk). > > But RelationGetIndexAttrBitmap is called for the table that contains the > index (and probably contains some other indexes too), not for one > specific index. So the bitmap is about the columns involved in *all* > indexes of the table ... > D'oh! Well, I did say I might be confused ... Yeah, that optimization is not possible, unfortunately. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [PATCH] Don't block HOT update by BRIN index
On 2021-Jul-12, Josef Šimánek wrote: > > 2) Do we actually need to calculate and store hotblockingattrs > > separately in RelationGetIndexAttrBitmap? It seems to me it's either > > NULL (with amhotblocking=false) or equal to indexattrs. So why not to > > just get rid of hotblockingattr and rd_hotblockingattr, and do something > > like > > > > case INDEX_ATTR_BITMAP_HOT_BLOCKING: > > return (amhotblocking) ? bms_copy(rel->rd_hotblockingattr) : NULL; > > > > I haven't tried, so maybe I'm missing something? > > relation->rd_indexattr is currently not used (at least I have not > found anything) for anything, except looking if other values are > already loaded. Oh, that's interesting. What this means is that INDEX_ATTR_BITMAP_ALL is no longer used; its uses must have all been replaced by something else. It seems the only one that currently exists is for HOT in heap_update, which this patch replaces with the new one. In a quick search, no external code depends on it, so I'd be inclined to just remove it ... I think a boolean is much simpler. Consider a table with 1600 columns :-) -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente"
Re: [CLOBBER_CACHE]Server crashed with segfault 11 while executing clusterdb
Amul Sul writes: > [ v5_Add-RelationGetSmgr-inline-function.patch ] Pushed with minor cosmetic adjustments. RelationCopyStorage() kind of gives me the willies. It's not really an smgr-level function, but we call it everywhere with smgr pointers that belong to relcache entries: /* copy main fork */ - RelationCopyStorage(rel->rd_smgr, dstrel, MAIN_FORKNUM, + RelationCopyStorage(RelationGetSmgr(rel), dstrel, MAIN_FORKNUM, rel->rd_rel->relpersistence); So that would fail hard if a relcache flush could occur inside that function. It seems impossible today, so I settled for just annotating the function to that effect. But it won't surprise me a bit if somebody breaks it in future due to not having read/understood the comment. regards, tom lane
Re: [PATCH] Don't block HOT update by BRIN index
On 7/12/21 11:02 PM, Alvaro Herrera wrote: > On 2021-Jul-12, Josef Šimánek wrote: > >>> 2) Do we actually need to calculate and store hotblockingattrs >>> separately in RelationGetIndexAttrBitmap? It seems to me it's either >>> NULL (with amhotblocking=false) or equal to indexattrs. So why not to >>> just get rid of hotblockingattr and rd_hotblockingattr, and do something >>> like >>> >>> case INDEX_ATTR_BITMAP_HOT_BLOCKING: >>> return (amhotblocking) ? bms_copy(rel->rd_hotblockingattr) : NULL; >>> >>> I haven't tried, so maybe I'm missing something? >> >> relation->rd_indexattr is currently not used (at least I have not >> found anything) for anything, except looking if other values are >> already loaded. > > Oh, that's interesting. What this means is that INDEX_ATTR_BITMAP_ALL > is no longer used; its uses must have all been replaced by something > else. It seems the only one that currently exists is for HOT in > heap_update, which this patch replaces with the new one. In a quick > search, no external code depends on it, so I'd be inclined to just > remove it ... > > I think a boolean is much simpler. Consider a table with 1600 columns :-) > I'm not sure how to verify no external code depends on that flag. I have no idea if there's a plausible use case for it, though. Even with 1600 columns the amount of wasted memory is only about 200B, which is not that bad I think. Not great, not terrible. OTOH most tables won't have any BRIN indexes, in which case indexattr and hotblockingattr are guaranteed to be exactly the same. So maybe that's something we could leverage - we need to calculate the "hot" bitmap, and in most cases we can use it for indexattr too. Maybe let's leave that for a separate patch, though? regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Support kerberos authentication for postgres_fdw
Greetings, * Magnus Hagander (mag...@hagander.net) wrote: > On Mon, Jul 12, 2021 at 5:43 AM Peifeng Qiu wrote: > > >As you note, this'd have to be restricted to superusers, which makes it > > >seem like a pretty bad idea. We really don't want to be in a situation > > >of pushing people to run day-to-day stuff as superuser. Yeah, having > > >access to kerberos auth sounds good on the surface, but it seems like > > >it would be a net loss in security because of that. > > > > I can imagine the use case would be a superuser creates the user > > mapping and foreign table, then grants access of foreign table to > > a normal user. This way the normal user can execute queries on the > > foreign table but can't access sensitive information in user mapping. > > > > The main purpose of this patch is to provide a simple way to do > > kerberos authentication with the least modification possible. > > But in this case, what dose Kerberos give over just using a password > based solution? It adds complexity, but what's teh actual gain? This is a bad idea. > > >ISTM the right way to do this would be using Kerberos delegation. That > > >is, the system would be set up so that the postgres service principal > > >is trusted for kerberos delegation and it would then pass through the > > >actual Kerberos authentication from the client. > > > > I agree this sounds like the ideal solution. If I understand it correctly, > > this approach requires both postgres servers to use same kerberos > > settings(kdc, realm, etc), and the FDW server can just "forward" > > necessary information to authenticate on behalf of the same user. > > I will spend some time to investigate it and reach out later. > > I don't actually know if they have to be in the same realm, I *think* > kerberos delegations work across trusted realms, but I'm not sure > about that. This is a good idea, and yes, delegation works just fine across realms if the environment is properly set up for cross-realm trust. Kerberos delegation is absolutely the way to go here. I don't think we should even be thinking of accepting something that requires users to put a bunch of keytab files on the PG server to allow that server to reach out to other servers... I'd be happy to work with someone on an effort to support Kerberos delegated credentials; it's been something that I've wanted to work on for a long time. Thanks, Stephen signature.asc Description: PGP signature
Re: What are exactly bootstrap processes, auxiliary processes, standalone backends, normal backends(user sessions)?
Greetings, * Bharath Rupireddy (bharath.rupireddyforpostg...@gmail.com) wrote: > I've always had a hard time distinguishing various types of > processes/terms used in postgres. I look at the source code every time > to understand them, yet I don't feel satisfied with my understanding. > I request any hacker (having a better idea than me) to help me with > what each different process does and how they are different from each > other? Of course, I'm clear with normal backends (user sessions), bg > workers, but the others need a bit more understanding. There was an effort to try to pull these things together because, yeah, it seems a bit messy. I'd suggest you take a look at: https://www.postgresql.org/message-id/flat/CAMN686FE0OdZKp9YPO=htC6LnA6aW4r-+jq=3q5raofqgw8...@mail.gmail.com Thanks, Stephen signature.asc Description: PGP signature
Re: [PATCH v3 1/1] Fix detection of preadv/pwritev support for OSX.
Thomas Munro writes: > Clearly there is a more general question though, which is "should we > buy into Apple's ABI management system or not", and I don't have a > strong opinion on that. Well, I definitely don't wish to clutter our core code with any explicit dependencies on MACOSX_DEPLOYMENT_TARGET. However, if we can work around the issue by switching from AC_REPLACE_FUNCS to AC_CHECK_DECLS, maybe we should just do that and quit arguing about it. It seems like there's not a lot of enthusiasm for my idea about installing a run-time probe, so I won't push for that. > One thing I do know is that > pthread_barrier_XXX changed from option to required in a recentish > POSIX update so I expect the question to come up again eventually. Yeah, we can expect that the issue will arise again, which is why I was so unhappy with the rather-invasive patches we started with. regards, tom lane
Re: proposal: possibility to read dumped table's name from file
> On 10 Jul 2021, at 17:47, Tomas Vondra wrote: > So if it was up to me, I'd go back to the original format or something close > it. So something like this: > > [+-] OBJECT_TYPE_PATTERN OBJECT_NAME_PATTERN That still leaves the parsing with quoting and escaping that needs to be done less trivial and more bespoke than what meets the eye, no? As mentioned upthread, I'm still hesitant to add a file format which deosn't have any version information of sorts for distinguishing it from when the inevitable "now wouldn't it be nice if we could do this too" patch which we all know will come. The amount of selectivity switches we have for pg_dump is an indication about just how much control users like, this will no doubt be subject to the same. -- Daniel Gustafsson https://vmware.com/
Re: proposal: possibility to read dumped table's name from file
On 7/13/21 12:08 AM, Daniel Gustafsson wrote: On 10 Jul 2021, at 17:47, Tomas Vondra wrote: So if it was up to me, I'd go back to the original format or something close it. So something like this: [+-] OBJECT_TYPE_PATTERN OBJECT_NAME_PATTERN That still leaves the parsing with quoting and escaping that needs to be done less trivial and more bespoke than what meets the eye, no? Yes, it'd require proper escaping/quoting of the fields/identifiers etc. As mentioned upthread, I'm still hesitant to add a file format which deosn't have any version information of sorts for distinguishing it from when the inevitable "now wouldn't it be nice if we could do this too" patch which we all know will come. The amount of selectivity switches we have for pg_dump is an indication about just how much control users like, this will no doubt be subject to the same. I'm not going to fight against some sort of versioning, but I think keeping the scope as narrow as possible would make it unnecessary. That is, let's stick to the original goal to allow passing filtering rules that would not fit on the command-line, and maybe let's make it a bit more flexible to support other object types etc. IMHO the filtering rules are simple enough to not really need elaborate versioning, and if a more advanced rule is proposed in the future it can be supported in the existing format (extra field, ...). Of course, maybe my imagination is not wild enough. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: proposal: possibility to read dumped table's name from file
On 2021-Jul-13, Tomas Vondra wrote: > I'm not going to fight against some sort of versioning, but I think keeping > the scope as narrow as possible would make it unnecessary. That is, let's > stick to the original goal to allow passing filtering rules that would not > fit on the command-line, and maybe let's make it a bit more flexible to > support other object types etc. > > IMHO the filtering rules are simple enough to not really need elaborate > versioning, and if a more advanced rule is proposed in the future it can be > supported in the existing format (extra field, ...). I don't understand why is versioning needed for this file. Surely we can just define some line-based grammar that's accepted by the current pg_dump[1] and that would satisfy the current need as well as allowing for extending the grammar in the future; even JSON or Windows-INI format (ugh?) if that's necessary to tailor the output file in some other way not covered by that. [1] your proposal of "[+-] OBJTYPE OBJIDENT" plus empty lines allowed plus lines starting with # are comments, seems plenty. Any line not following that format would cause an error to be thrown. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: [PATCH] Don't block HOT update by BRIN index
On 2021-Jul-12, Tomas Vondra wrote: > I'm not sure how to verify no external code depends on that flag. I have > no idea if there's a plausible use case for it, though. But we don't *have* to, do we? -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/ "Tiene valor aquel que admite que es un cobarde" (Fernandel)
Re: [PATCH] Don't block HOT update by BRIN index
po 12. 7. 2021 v 23:15 odesílatel Tomas Vondra napsal: > > > > On 7/12/21 11:02 PM, Alvaro Herrera wrote: > > On 2021-Jul-12, Josef Šimánek wrote: > > > >>> 2) Do we actually need to calculate and store hotblockingattrs > >>> separately in RelationGetIndexAttrBitmap? It seems to me it's either > >>> NULL (with amhotblocking=false) or equal to indexattrs. So why not to > >>> just get rid of hotblockingattr and rd_hotblockingattr, and do something > >>> like > >>> > >>> case INDEX_ATTR_BITMAP_HOT_BLOCKING: > >>> return (amhotblocking) ? bms_copy(rel->rd_hotblockingattr) : NULL; > >>> > >>> I haven't tried, so maybe I'm missing something? > >> > >> relation->rd_indexattr is currently not used (at least I have not > >> found anything) for anything, except looking if other values are > >> already loaded. > > > > Oh, that's interesting. What this means is that INDEX_ATTR_BITMAP_ALL > > is no longer used; its uses must have all been replaced by something > > else. It seems the only one that currently exists is for HOT in > > heap_update, which this patch replaces with the new one. In a quick > > search, no external code depends on it, so I'd be inclined to just > > remove it ... > > > > I think a boolean is much simpler. Consider a table with 1600 columns :-) > > > > I'm not sure how to verify no external code depends on that flag. I have > no idea if there's a plausible use case for it, though. I tried GitHub search before to ensure at least it is not a widely used "API". There were no results outside of PostgreSQL code itself in first 10 pages of results. > Even with 1600 columns the amount of wasted memory is only about 200B, > which is not that bad I think. Not great, not terrible. > > OTOH most tables won't have any BRIN indexes, in which case indexattr > and hotblockingattr are guaranteed to be exactly the same. So maybe > that's something we could leverage - we need to calculate the "hot" > bitmap, and in most cases we can use it for indexattr too. > > Maybe let's leave that for a separate patch, though? > > > regards > > -- > Tomas Vondra > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company
Re: Add proper planner support for ORDER BY / DISTINCT aggregates
Em seg., 12 de jul. de 2021 às 09:04, David Rowley
escreveu:
> On Sun, 13 Jun 2021 at 03:07, David Rowley wrote:
> >
> > Please find attached my WIP patch. It's WIP due to what I mentioned
> > in the above paragraph and also because I've not bothered to add JIT
> > support for the new expression evaluation steps.
>
> I've split this patch into two parts.
>
Hi, I'll take a look.
> 0001 Adds planner support for ORDER BY aggregates.
>
/* Normal transition function without ORDER BY / DISTINCT. */
Is it possible to avoid entering to initialize args if 'argno >=
pertrans->numTransInputs'?
Like this:
if (!pertrans->aggsortrequired && argno < pertrans->numTransInputs)
And if argos is '>' that pertrans->numTransInputs?
The test shouldn't be, inside the loop?
+ /*
+ * Don't initialize args for any ORDER BY clause that might
+ * exist in a presorted aggregate.
+ */
+ if (argno >= pertrans->numTransInputs)
+ break;
I think that or can reduce the scope of variable 'sortlist' or simply
remove it?
a)
+ /* Determine pathkeys for aggregate functions with an ORDER BY */
+ if (parse->groupingSets == NIL && root->numOrderedAggs > 0 &&
+ (qp_extra->groupClause == NIL || root->group_pathkeys))
+ {
+ ListCell *lc;
+ List *pathkeys = NIL;
+
+ foreach(lc, root->agginfos)
+ {
+ AggInfo*agginfo = (AggInfo *) lfirst(lc);
+ Aggref *aggref = agginfo->representative_aggref;
+ List *sortlist;
+
or better
b)
+ /* Determine pathkeys for aggregate functions with an ORDER BY */
+ if (parse->groupingSets == NIL && root->numOrderedAggs > 0 &&
+ (qp_extra->groupClause == NIL || root->group_pathkeys))
+ {
+ ListCell *lc;
+ List *pathkeys = NIL;
+
+ foreach(lc, root->agginfos)
+ {
+ AggInfo*agginfo = (AggInfo *) lfirst(lc);
+ Aggref *aggref = agginfo->representative_aggref;
+
+ if (AGGKIND_IS_ORDERED_SET(aggref->aggkind))
+ continue;
+
+ /* DISTINCT aggregates not yet supported by the planner */
+ if (aggref->aggdistinct != NIL)
+ continue;
+
+ if (aggref->aggorder == NIL)
+ continue;
+
+ /*
+ * Find the pathkeys with the most sorted derivative of the first
+ * Aggref. For example, if we determine the pathkeys for the first
+ * Aggref to be {a}, and we find another with {a,b}, then we use
+ * {a,b} since it's useful for more Aggrefs than just {a}. We
+ * currently ignore anything that might have a longer list of
+ * pathkeys than the first Aggref if it is not contained in the
+ * pathkeys for the first agg. We can't practically plan for all
+ * orders of each Aggref, so this seems like the best compromise.
+ */
+ if (pathkeys == NIL)
+ {
+ pathkeys = make_pathkeys_for_sortclauses(root, aggref->aggorder ,
+ aggref->args);
+ aggref->aggpresorted = true;
+ }
+ else
+ {
+ List *pathkeys2 = make_pathkeys_for_sortclauses(root,
+ aggref->aggorder,
+ aggref->args);
> 0002 is a WIP patch for DISTINCT support. This still lacks JIT
> support and I'm still not certain of the best where to store the
> previous value or tuple to determine if the current one is distinct
> from it.
>
In the patch 0002, I think that can reduce the scope of variable 'aggstate'?
+ EEO_CASE(EEOP_AGG_PRESORTED_DISTINCT_SINGLE)
+ {
+ AggStatePerTrans pertrans = op->d.agg_presorted_distinctcheck.pertrans;
+ Datum value = pertrans->transfn_fcinfo->args[1].value;
+ bool isnull = pertrans->transfn_fcinfo->args[1].isnull;
+
+ if (!pertrans->haslast ||
+ pertrans->lastisnull != isnull ||
+ !DatumGetBool(FunctionCall2Coll(&pertrans->equalfnOne,
+ pertrans->aggCollation,
+ pertrans->lastdatum, value)))
+ {
+ if (pertrans->haslast && !pertrans->inputtypeByVal)
+ pfree(DatumGetPointer(pertrans->lastdatum));
+
+ pertrans->haslast = true;
+ if (!isnull)
+ {
+ AggState *aggstate = castNode(AggState, state->parent);
+
+ /*
+ * XXX is it worth having a dedicted ByVal version of this
+ * operation so that we can skip switching memory contexts
+ * and do a simple assign rather than datumCopy below?
+ */
+ MemoryContext oldContext;
+
+ oldContext =
MemoryContextSwitchTo(aggstate->curaggcontext->ecxt_per_tuple_memory);
What do you think?
regards,
Ranier Vilela
Re: proposal: possibility to read dumped table's name from file
Alvaro Herrera writes: > [1] your proposal of "[+-] OBJTYPE OBJIDENT" plus empty lines allowed > plus lines starting with # are comments, seems plenty. Any line not > following that format would cause an error to be thrown. I'd like to see some kind of keyword on each line, so that we could extend the command set by adding new keywords. As this stands, I fear we'd end up using random punctuation characters in place of [+-], which seems pretty horrid from a readability standpoint. I think that this file format should be designed with an eye to allowing every, or at least most, pg_dump options to be written in the file rather than on the command line. I don't say we have to *implement* that right now; but if the format spec is incapable of being extended to meet requests like that one, I think we'll regret it. This line of thought suggests that the initial commands ought to match the existing include/exclude switches, at least approximately. Hence I suggest include table PATTERN exclude table PATTERN which ends up being the above but with words not [+-]. regards, tom lane
Bogus HAVE_DECL_FOO entries in msvc/Solution.pm
Autoconf's AC_CHECK_DECLS always defines HAVE_DECL_whatever as 1 or 0, but some of the entries in msvc/Solution.pm show such symbols as "undef" instead. Shouldn't we fix it as per attached? This is probably only cosmetic at the moment, but it could bite us someday if someone wrote a complex conditional using one of these symbols. These apparently-bogus values date to Peter's 8f4fb4c64, which created that table; but AFAICS it was just faithfully emulating the previous confused state of affairs. regards, tom lane diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm index 294b968dcd..cfda5ac185 100644 --- a/src/tools/msvc/Solution.pm +++ b/src/tools/msvc/Solution.pm @@ -239,18 +239,18 @@ sub GenerateFiles HAVE_CRYPTO_LOCK => undef, HAVE_DECL_FDATASYNC=> 0, HAVE_DECL_F_FULLFSYNC => 0, - HAVE_DECL_LLVMCREATEGDBREGISTRATIONLISTENER => undef, - HAVE_DECL_LLVMCREATEPERFJITEVENTLISTENER=> undef, + HAVE_DECL_LLVMCREATEGDBREGISTRATIONLISTENER => 0, + HAVE_DECL_LLVMCREATEPERFJITEVENTLISTENER=> 0, HAVE_DECL_LLVMGETHOSTCPUNAME=> 0, HAVE_DECL_LLVMGETHOSTCPUFEATURES=> 0, HAVE_DECL_LLVMORCGETSYMBOLADDRESSIN => 0, - HAVE_DECL_POSIX_FADVISE => undef, + HAVE_DECL_POSIX_FADVISE => 0, HAVE_DECL_PREADV=> 0, HAVE_DECL_PWRITEV => 0, HAVE_DECL_RTLD_GLOBAL => 0, HAVE_DECL_RTLD_NOW => 0, - HAVE_DECL_STRLCAT => undef, - HAVE_DECL_STRLCPY => undef, + HAVE_DECL_STRLCAT => 0, + HAVE_DECL_STRLCPY => 0, HAVE_DECL_STRNLEN => 1, HAVE_DECL_STRTOLL => 1, HAVE_DECL_STRTOULL => 1,
Re: [PATCH] Pull general SASL framework out of SCRAM
On Sun, 2021-07-11 at 13:16 +0900, Michael Paquier wrote: > On Fri, Jul 09, 2021 at 11:31:48PM +, Jacob Champion wrote: > > On Thu, 2021-07-08 at 16:27 +0900, Michael Paquier wrote: > > > + * outputlen: The length (0 or higher) of the client response > > > buffer, > > > + * invalid if output is NULL. > > > > nitpick: maybe "ignored" instead of "invalid"? > > Thanks, applied as 44bd012 after using your suggestion. Thanks! > Another thing I noticed after more review is that the check in > fe-auth.c to make sure that a message needs to be generated if the > exchange is not completed yet has no need to depend on "success", only > "done". Ah, right. I think the (!done && !success) case is probably indicative of an API smell, but that's probably something to clean up in a future pass. --Jacob
Re: enable_resultcache confusion
On Tue, 13 Jul 2021 at 01:38, Tom Lane wrote: > > David Rowley writes: > > On Mon, 12 Jul 2021 at 03:22, Justin Pryzby wrote: > >> |This is useful if only a small percentage of rows is checked on > >> |the inner side and is controlled by >> |linkend="guc-enable-resultcache"/>. > > > You might be right there, but I'm not too sure if I changed that that > > it might cause a mention of the rename to be missed in the changes > > since beta2 notes. > > You need to change it, because IIUC that will be a dangling > cross-reference, causing the v14 docs to fail to build at all. Good point. I'll adjust that for PG14. I plan on pushing the patch to master and PG14 in 24 hours time. If anyone is still on the fence or wishes to object to the name, please let it be known before then. David
Re: proposal - psql - use pager for \watch command
On Tue, Jul 13, 2021 at 8:20 AM Pavel Stehule wrote: > po 12. 7. 2021 v 18:12 odesílatel vignesh C napsal: >> Thanks for fixing the comments, CFbot also passes for the same. I have >> changed the status back to "Ready for Committer". > > I tested this version with the last release and with a developing version of > pspg, and it works very well. Pushed, after retesting on macOS (with the fixed pspg that has by now arrived in MacPorts), FreeBSD and Linux. Thanks! I'm using this to monitor system views when demoing new features in development, it's nice. Of course, I don't like the default theme, it's a bit too MS-DOS/Norton for my taste, but the quieter themes are good :-)
Re: Fix comments of heap_prune_chain()
On 2021-Jul-12, Alvaro Herrera wrote: > On 2021-Jul-12, ikeda...@oss.nttdata.com wrote: > > > While I’m reading source codes related to vacuum, I found comments which > > don’t seem to fit the reality. I think the commit[1] just forgot to fix > > them. > > What do you think? > > > > [1] > > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=dc7420c2c9274a283779ec19718d2d16323640c0 > > That sounds believable, but can you be more specific? Oh, apologies, I didn't realize there was an attachment. That seems specific enough :-) In my defense, the archives don't show the attachment either: https://www.postgresql.org/message-id/5CB29811-2B1D-4244-8DE2-B1E02495426B%40oss.nttdata.com I think we've seen this kind of problem before -- the MIME structure of the message is quite unusual, which is why neither my MUA nor the archives show it. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Los trabajadores menos efectivos son sistematicamente llevados al lugar donde pueden hacer el menor daño posible: gerencia." (El principio Dilbert)
Re: Incorrect usage of strtol, atoi for non-numeric junk inputs
On Fri, Jul 09, 2021 at 04:50:28PM +0900, Kyotaro Horiguchi wrote:
> At Fri, 9 Jul 2021 10:29:07 +0900, Michael Paquier
> wrote in
>> Er, wait. We've actually allowed negative values for pg_ctl
>> --timeout or the subcommand kill!?
>
> --timeout accepts values less than 1, which values cause the command
> ends with the timeout error before checking for the ending state. Not
> destructive but useless as a behavior. We have two choices here. One
> is simply inhibiting zero or negative timeouts, and another is
> allowing zero as timeout and giving it the same meaning to
> --no-wait. The former is strictly right behavior but the latter is
> casual and convenient. I took the former way in this version.
Yeah, that's not useful.
>> This one in pg_upgrade is incomplete. Perhaps the missing comment
>> should tell that negative job values are checked later on?
>
> Zero or negative job numbers mean non-parallel mode and don't lead to
> an error. If we don't need to preserve that behavior (I personally
> don't think it is ether useful and/or breaks someone's existing
> usage.), it is sensible to do range-check here.
Hmm. It would be good to preserve some compatibility here, but I can
see more benefits in being consistent between all the tools, and make
people aware that such commands are not generated more carefully.
> case 'j':
> -opts.jobs = atoi(optarg);
> -if (opts.jobs < 1)
> +errno = 0;
> +opts.jobs = strtoint(optarg, &endptr, 10);
> +if (*endptr || errno == ERANGE || opts.jobs < 1)
> {
> pg_log_error("number of parallel jobs must be at least
> 1");
> exit(1);
specifying a value that triggers ERANGE could be confusing for values
higher than INT_MAX with pg_amcheck --jobs:
$ pg_amcheck --jobs=40
pg_amcheck: error: number of parallel jobs must be at least 1
I think that this should be reworded as "invalid number of parallel
jobs \"$s\"", or "number of parallel jobs must be in range %d..%d".
Perhaps we could have a combination of both? Using the first message
is useful with junk, non-numeric values or trailing characters. The
second is useful to make users aware that the value is numeric, but
not quite right.
> --- a/src/bin/pg_checksums/pg_checksums.c
> case 'f':
> -if (atoi(optarg) == 0)
> +errno = 0;
> +if (strtoint(optarg, &endptr, 10) == 0
> +|| *endptr || errno == ERANGE)
> {
> pg_log_error("invalid filenode specification, must be
> numeric: %s", optarg);
> exit(1);
The confusion is equal here with pg_checksums -f:
$ ./pg_checksums --f 40
pg_checksums: error: invalid filenode specification, must be numeric: 4
We could say "invalid file specification: \"%s\"". Another idea to be
crystal-clear about the range requirements is to use that:
"file specification must be in range %d..%d"
> @@ -587,8 +602,10 @@ main(int argc, char **argv)
>
> case 8:
> have_extra_float_digits = true;
> -extra_float_digits = atoi(optarg);
> -if (extra_float_digits < -15 || extra_float_digits > 3)
> +errno = 0;
> +extra_float_digits = strtoint(optarg, &endptr, 10);
> +if (*endptr || errno == ERANGE ||
> +extra_float_digits < -15 || extra_float_digits > 3)
> {
> pg_log_error("extra_float_digits must be in range
> -15..3");
> exit_nicely(1);
Should we take this occasion to reduce the burden of translators and
reword that as "%s must be in range %d..%d"? That could be a separate
patch.
> case 'p':
> -if ((old_cluster.port = atoi(optarg)) <= 0)
> -pg_fatal("invalid old port number\n");
> +errno = 0;
> +if ((old_cluster.port = strtoint(optarg, &endptr, 10)) <= 0
> ||
> +*endptr || errno == ERANGE)
> +pg_fatal("invalid old port number %s\n", optarg);
> break;
You may want to use columns here, or specify the port range:
"invalid old port number: %s" or "old port number must be in range
%d..%d".
> case 'P':
> -if ((new_cluster.port = atoi(optarg)) <= 0)
> -pg_fatal("invalid new port number\n");
> +errno = 0;
> +if ((new_cluster.port = strtoint(optarg, &endptr, 10)) <= 0
> ||
> +*endptr || errno == ERANGE)
> +pg_fatal("invalid new port number %s\n", optarg);
> break;
Ditto.
> +if (*endptr || errno == ERANGE || concurrentCons <= 0)
> {
> -pg_log_error("number of parallel jobs must be at least
>
