Re: [HACKERS] pg_basebackup --progress output for batch execution
On Thu, Nov 09, 2017 at 03:55:36PM -0800, Jeff Janes wrote: > > I think I agree with Arthur that I'd rather have the decision made by > inspecting whether output is going to a tty, rather than by adding another > command line option. But maybe that is not detected robustly enough across > all platforms and circumstances? > isatty() is used within Postgres code already (for example, pg_upgrade/util.c). However, it seems that on Windows isatty() is deprecated and it is recommended to use _isatty(). Moreover, on Windows it can give false positive result [1], if I'm not mistaken: > The _isatty function determines whether fd is associated with a character > device (a terminal, console, printer, or serial port). 1 - https://msdn.microsoft.com/en-us/library/f4s0ddew(v=vs.140).aspx -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_basebackup --progress output for batch execution
Hello,
On Sun, Oct 01, 2017 at 04:49:17PM -0300, Martin Marques wrote:
> Updated patch with documentation of the new option.
>
I have checked the patch.
The patch is applied and compiled correctly without any errors. Tests passed.
The documentation doesn't have errors too.
I have a little suggestion. Maybe insert new line without any additional
parameters? We can check that stderr is not terminal using isatty().
The code could become:
if (!isatty(fileno(stderr)))
fprintf(stderr, "\n");
else
fprintf(stderr, "\r");
Also it could be good to not insert new line after progress:
if (showprogress)
{
progress_report(PQntuples(res), NULL, true);
/* if (!batchmode) */
/* or */
if (isatty(fileno(stderr)))
fprintf(stderr, "\n"); /* Need to move to next line */
}
Otherwise we have an extra empty line:
pg_basebackup: initiating base backup, waiting for checkpoint to complete
pg_basebackup: checkpoint completed
pg_basebackup: write-ahead log start point: 0/1C28 on timeline 1
pg_basebackup: starting background WAL receiver
pg_basebackup: created temporary replication slot "pg_basebackup_19635"
0/183345 kB (0%), 0/1 tablespace (data_repl/backup_label )
183355/183355 kB (100%), 0/1 tablespace (data_repl/global/pg_control)
183355/183355 kB (100%), 1/1 tablespace
pg_basebackup: write-ahead log end point: 0/1CF8
pg_basebackup: waiting for background process to finish streaming ...
pg_basebackup: base backup completed
--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Generic type subscripting
Thank you for fixing.
On Tue, Nov 07, 2017 at 09:00:43PM +0100, Dmitry Dolgov wrote:
> > > +Datum
> > > +custom_subscripting_parse(PG_FUNCTION_ARGS)
> > > +{
> > > + boolisAssignment = PG_GETARG_BOOL(0);
> >
> > Here isAssignment is unused variable, so it could be removed.
>
> In this case I disagree - the purpose of these examples is to show
> everything
> you can use. So I just need to come up with some example that involves
> `isAssignment`.
Understood.
>
> > > + scratch->d.sbsref.eval_finfo = eval_finfo;
> > > + scratch->d.sbsref.nested_finfo = nested_finfo;
> > > +
> > As I mentioned earlier we need assigning eval_finfo and nested_finfo only
> for EEOP_SBSREF_OLD, EEOP_SBSREF_ASSIGN and EEOP_SBSREF_FETCH steps.
> > Also they should be assigned before calling ExprEvalPushStep(), not
> after. Otherwise some bugs may appear in future.
>
> I'm really confused about this one. Can you tell me the exact line numbers?
> Because if I remove any of these lines "blindly", tests are failing.
Field scratch->d is union. Its fields should be changed only before calling
ExprEvalPushStep(), which copies 'scratch'. To be more specific I attached the
patch 0005-Fix-ExprEvalStep.patch, which can be applyed over your patches.
Some other notes are below.
> class="parameter">type_modifier_output_function and
> - analyze_function
> + analyze_function,
> + subscripting_parse_function
> + subscripting_assign_function
> + subscripting_fetch_function
I think you forgot commas and conjunction 'and'.
> + The optional
> + subscripting_parse_function,
> + subscripting_assign_function
> + subscripting_fetch_function
> + contains type-specific logic for subscripting of the data type.
Here you forgot comma or 'and'. Also 'contain' should be used instead
'contains'.
It seems that in the following you switched descriptions:
> + class="parameter">subscripting_assign_function
> +
> +
> + The name of a function that contains type-specific subscripting logic
> for
> + fetching an element from the data type.
> +
subscripting_assign_function is used for updating.
> + class="parameter">subscripting_fetch_function
> +
> +
> + The name of a function that contains type-specific subscripting logic
> for
> + updating an element in the data type.
> +
subscripting_fetch_function is used for fetching.
I have a little complain about how ExprEvalStep gets resvalue. resvalue is
assigned in one place (within ExecEvalSubscriptingRefFetch(),
ExecEvalSubscriptingRefAssign()), resnull is assigned in another place (within
jsonb_subscript_fetch(), jsonb_subscript_assign()). I'm not sure that it is a
good idea, but it is not critical, it is just complaint.
After your fixing I think we should wait for opinion of senior community
members and mark the patch as 'Ready for Commiter'. Maybe I will do more tests
and try to implement subscripting to another type.
--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index c275387319..42a0d31c31 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -2350,9 +2350,6 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *aref, PlanState
fmgr_info(aref->refnestedfunc, nested_finfo);
}
- scratch->d.sbsref.eval_finfo = eval_finfo;
- scratch->d.sbsref.nested_finfo = nested_finfo;
-
/* Fill constant fields of SubscriptingRefState */
arefstate->isassignment = isAssignment;
arefstate->refelemtype = aref->refelemtype;
@@ -2377,9 +2374,6 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *aref, PlanState
scratch->d.jump.jumpdone = -1; /* adjust later */
ExprEvalPushStep(state, scratch);
- scratch->d.sbsref.eval_finfo = eval_finfo;
- scratch->d.sbsref.nested_finfo = nested_finfo;
-
adjust_jumps = lappend_int(adjust_jumps,
state->steps_len - 1);
}
@@ -2425,9 +2419,6 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *aref, PlanState
scratch->d.sbsref_subscript.jumpdone = -1; /* adjust later */
ExprEvalPushStep(state, scratch);
- scratch->d.sbsref.eval_finfo = eval_finfo;
- scratch->d.sbsref.nested_finfo = nested_finfo;
-
adjust_jumps = lappend_int(adjust_jumps,
state->steps_len - 1);
i++;
@@ -2462,9 +2453,6 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *aref, PlanState
scratch->d.sbsref_subscript.jumpdone = -1; /* adjust later */
ExprEvalPus
Re: [HACKERS] [PATCH] Generic type subscripting
On Sun, Oct 29, 2017 at 10:56:19PM +0100, Dmitry Dolgov wrote:
>
> So, here is the new version of patch that contains modifications we've
> discussed, namely:
>
> * store oids of `parse`, `fetch` and `assign` functions
>
> * introduce dependencies from a data type
>
> * as a side effect of previous two I also eliminated some unnecessary
> arguments
> in `parse` function.
Thank you for new version of the patch.
There are some notes.
Documentation
-
Documentation is compiled. But there are warnings about end-tags. Now it is
necessary to have full named end-tags:
=# make -C doc/src/sgml check
/usr/sbin/onsgmls:json.sgml:574:20:W: empty end-tag
...
Documentation is out of date:
- catalogs.sgml needs to add information about additional pg_type fields
- create_type.sgml needs information about subscripting_parse,
subscripting_assign and subscripting_fetch options
- xsubscripting.sgml is out of date
Code
I think it is necessary to check Oids of subscripting_parse,
subscripting_assign, subscripting_fetch. Maybe within TypeCreate().
Otherwise next cases possible:
=# CREATE TYPE custom (
internallength = 8,
input = custom_in,
output = custom_out,
subscripting_parse = custom_subscripting_parse);
=# CREATE TYPE custom (
internallength = 8,
input = custom_in,
output = custom_out,
subscripting_fetch = custom_subscripting_fetch);
Are all subscripting_* fields mandatory? If so if user provided at least one of
them then all fields should be provided.
Should all types have support assigning via subscript? If not then
subscripting_assign parameter is optional.
> +Datum
> +jsonb_subscript_parse(PG_FUNCTION_ARGS)
> +{
> + boolisAssignment = PG_GETARG_BOOL(0);
and
> +Datum
> +custom_subscripting_parse(PG_FUNCTION_ARGS)
> +{
> + boolisAssignment = PG_GETARG_BOOL(0);
Here isAssignment is unused variable, so it could be removed.
> +
> + scratch->d.sbsref.eval_finfo = eval_finfo;
> + scratch->d.sbsref.nested_finfo = nested_finfo;
> +
As I mentioned earlier we need assigning eval_finfo and nested_finfo only for
EEOP_SBSREF_OLD, EEOP_SBSREF_ASSIGN and EEOP_SBSREF_FETCH steps.
Also they should be assigned before calling ExprEvalPushStep(), not after.
Otherwise some bugs may appear in future.
> - ArrayRef *aref = makeNode(ArrayRef);
> + NodeTag sbstag = nodeTag(src_expr);
> + Size nodeSize = sizeof(SubscriptingRef);
> + SubscriptingRef *sbsref = (SubscriptingRef *) newNode(nodeSize,
> sbstag);
Is there necessity to use newNode() instead using makeNode(). The previous code
was shorter.
There is no changes in execnodes.h except removed line. So I think execnodes.h
could be removed from the patch.
>
> I'm going to make few more improvements, but in the meantime I hope we can
> continue to review the patch.
I will wait.
--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Generic type subscripting
On Wed, Sep 20, 2017 at 09:35:06AM -0400, Peter Eisentraut wrote: > > The difference is that those create associations between two separate > objects (cast: type1 <-> type2, transform: type <-> language). A > subscripting is just a property of a type. > > -- > Peter Eisentraut http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services Yes, indeed. I agree. As a conclusion: - additional field are needed to pg_type for *_fetch and *_assign functions to solve dependency problem - ALTER TYPE requires a modification to add or drop subscripting from existing type (I am not sure that it should be done by this patch, maybe better to do it by a separate patch) -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Generic type subscripting
On Tue, Sep 19, 2017 at 09:01:57PM -0400, Peter Eisentraut wrote: > Would you mind posting a summary of how you go here? There were several points here to me: - it is necessary to solve the dependency problem (it can be solved also by adding several oid fields to the pg_type) - users may want to add subscripting to their existing type, which already created in their database, or drop subscripting from existing type (it cannot be done by CREATE TYPE) - other type related functionalities have their CREATE command and system catalog table. For example, CREATE CAST, CREATE TRANSFORM (this is a weakest point I think, mostly because of several casts and transforms can be defined to one type, and only one subscripting can be defined to one type). > Superficially, it seems like this sort of feature should be handled by a > couple of type attributes and pg_type columns. But you are talking > about a new system catalog and new DDL, and it's not clear to me how you > got here. > > -- > Peter Eisentraut http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services You are right of course. It can be handled by oid columns of *_parse, *_extract and *_assign functions. Also it is clear to me now that the second point can be handled by ALTER TYPE command. The command should be modified to handle it of course. And it can be modified by a separate patch later. I want to emphasize that I don't insist on CREATE SUBSCRIPTING command. The patch brings important functionality and I don't want to be a person who blocked it from commiting. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Generic type subscripting
On Mon, Sep 18, 2017 at 12:25:04PM +0200, Dmitry Dolgov wrote: > > I think it would be good to add new catalog table. It may be named as > pg_type_sbs or pg_subscripting (second is better I think). > > This table may have the fields: > > - oid > > - sbstype > > - sbsinit > > - sbsfetch > > - sbsassign > > What is `sbstype`? 'sbstype' is oid of type from pg_type for which subscripting is created. I.e. pg_type may not have the 'typsubsparse' field. > > And command 'CREATE SUBSCRIPTING' should add an entry to the > pg_subscripting table. It also adds entries to the pg_depend table: > dependency between pg_type and pg_subscripting, dependency between pg_type > and pg_proc. > > 'DROP SUBSCRIPTING' should drop this entries, it should not drop init > function. > > Why we should keep those subscripting functions? From my understanding > they're > totally internal and useless without subscripting context. > Other similar commands do not drop related functions. For example 'DROP CAST' do not drop a function used to perform a cast. > It also adds entries to the pg_depend table: dependency between pg_type and > pg_subscripting, dependency between pg_type and pg_proc. Here was a typo from me. Last entry is dependency between pg_subscripting and pg_proc. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Generic type subscripting
On Mon, Sep 18, 2017 at 10:31:54AM +0200, Dmitry Dolgov wrote: > Just to clarify, do you mean that `CREATE SUBSCRIPTING FOR` would only make > a > dependency record? In this case `DROP SUBSCRIPTING FOR` actually means just > drop an init function. I think it would be good to add new catalog table. It may be named as pg_type_sbs or pg_subscripting (second is better I think). This table may have the fields: - oid - sbstype - sbsinit - sbsfetch - sbsassign And command 'CREATE SUBSCRIPTING' should add an entry to the pg_subscripting table. It also adds entries to the pg_depend table: dependency between pg_type and pg_subscripting, dependency between pg_type and pg_proc. 'DROP SUBSCRIPTING' should drop this entries, it should not drop init function. According to the Tom's comment the syntax can be modified in the following way: CREATE SUBSCRIPTING FOR type_namei ( INITFUNC = subscripting_init_func FETCHFUNC = subscripting_fetch_func ASSIGNFUNC = subscripting_assign_func ) DROP SUBSCRIPTING FOR type_name -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Generic type subscripting
On Sun, Sep 17, 2017 at 12:27:58AM +0200, Dmitry Dolgov wrote: > spite of what form this step will be. Maybe it's possible to make something > like `CREATE FUNCTION ... FOR SUBSCRIPTING`, then verify that assign/extract > functions are presented and notify user if he missed them (but I would > rather > not do this unless it's really necessary, since it looks like an overkill). > > But I'm open to any suggestions, do you have something in mind? I have put some thought into it. What about the following syntax? CREATE SUBSCRIPTING FOR type_name INITFUNC = subscripting_init_func FETCHFUNC = subscripting_fetch_func ASSIGNFUNC = subscripting_assign_func DROP SUBSCRIPTING FOR type_name But I am not if the community will like such syntax. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Generic type subscripting
On Fri, Sep 15, 2017 at 10:02:00PM +0200, Dmitry Dolgov wrote: > > So, I've implemented a patch for that in form of a `DEPENDS ON` syntax for > creating a function. In my opinion, 'DEPENDS ON' syntax is not actually appropriate here. It also looks like a not very good hack to me. Moreover user can implement subscripting to its own type without using 'DEPENDS ON' syntax. And he will face the bug mentioned above too. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Generic type subscripting
On Thu, Sep 07, 2017 at 10:49:54PM +0200, Dmitry Dolgov wrote: > On 29 August 2017 at 22:42, Dmitry Dolgov <9erthali...@gmail.com> wrote: > > > > To make a review little bit easier I've divided the patch into a few > smaller parts. > > Apparently I forgot about subscripting for the name data type, so here is a > small update of the patch. Thank you for rebasing the patch! PostgreSQL and documentation with the patch compiles without any errors. All regression tests passed. But honestly I still cannot say that I agree with *_extract() and *_assign() functions creation way. For example, there is no entry in pg_depend for them (related with pg_type entry). Because there is no such entry, there is the following bug: 1 - make and install src/tutorial 2 - run src/tutorial/subscripting.sql 3 - run: =# drop function custom_subscripting_extract(internal); 4 - and we get the error: =# select data[0] from test_subscripting; ERROR: function 0x55deb7911bfd returned NULL But of course it is only my opinion and I could be wrong. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PROPOSAL] Text search configuration extension
Hello,
On Fri, Aug 18, 2017 at 03:30:38PM +0300, Aleksandr Parfenov wrote:
> Hello hackers!
>
> I'm working on a new approach in text search configuration and want to
> share my thought with community in order to get some feedback and maybe
> some new ideas.
>
There are several cases, where the new syntax could be useful:
https://www.postgresql.org/message-id/4733b65a.9030...@students.mimuw.edu.pl
Firstly check is lexeme stopword or not, and only then normalize it.
https://www.postgresql.org/message-id/c6851b7e-da25-3d8e-a5df-022c395a11b4%40postgrespro.ru
Support union of outputs of several dictionaries.
https://www.postgresql.org/message-id/46D57E6F.8020009%40enterprisedb.com
Support of chain of dictionaries using MAP BY operator.
The basic idea of the approach is to bring to a user more control of text
search configurations without writing additional or modifing existing
dictionaries.
> ALTER TEXT SEARCH CONFIGURATION en_de_search ADD MAPPING FOR asciiword,
> word WITH
> CASE
>WHEN english_hunspell IS NOT NULL THEN english_hunspell
>WHEN german_hunspell IS NOT NULL THEN german_hunspell
>ELSE
> -- stem dictionaries can't be used for language detection
> english_stem UNION german_stem
> END;
For example, the configuration mentioned above will bring the following results:
=# select d @@ q, d, q from to_tsvector('german_hunspell', 'Dieser Hund wollte
ihn jedoch nicht nach Hause begleiten') d, to_tsquery('en_de_search', 'hause')
q;
?column? | d |q
--+--+--
t| 'begleiten':9 'hausen':8 'hund':2 'jedoch':5 | 'hausen'
(1 row)
This configuration is useful when a query language is unknown.
Best regards,
--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Regressions failures with libxml2 on ArchLinux
Hello, On Sat, Aug 12, 2017 at 08:46:38PM +0900, Michael Paquier wrote: > I can notice as well that no buildfarm machines are running ArchLinux > now, so that's one reason why this got undetected until now. My own > machines running Archlinux ARM have been unplugged for a full month, > and I can see the failure there. They are not able to report back to > the buildfarm, but that's a separate issue, visibly that's an access > permission. > > I have not investigated much this problem yet, but has somebody else > seen those diffs? > I have libxml2 2.9.4+96+gfb56f80e-1 on my Archlinux. All regression tests passed without any fails. I ran check and installcheck commands. Of course my environment is not completely match to your environment. It could be a reason why we have different results. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] List of hostaddrs not supported
Hello, 2017-07-10 12:30 GMT+03:00 Heikki Linnakangas : > > > I just remembered that this was still pending. I made the documentation > changes, and committed this patch now. > > We're uncomfortably close to wrapping the next beta, later today, but I > think it's better to get this into the hands of people testing this, rather > than wait for the next beta. I think the risk of breaking something that > used to work is small. > > I get this warning during compilation using gcc 7.1.1 20170621: > fe-connect.c:1100:61: warning: comparison between pointer and zero character constant [-Wpointer-compare] > conn->connhost[i].host != NULL && conn->connhost[i].host != '\0') -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Re: [HACKERS] [PATCH] Generic type subscripting
On Wednesday, 10 May 2017 23:43:10 MSK, Dmitry Dolgov wrote: > So, a few words about current state of the patch: > > * after a lot of serious improvements general design of this feature is > agreeable > > * we introduced a lot of small changes to polish it > > * I rebased the patch on the latest version of master, so you can take a > look at it again > > As always, any feedback is welcome. Hello, Can you rebase the patch please? It is not applyed now. I think it is because of pgindent. > + > + scratch->d.sbsref.eval_finfo = eval_finfo; > + scratch->d.sbsref.nested_finfo = nested_finfo; > + Also I have noticed that assigning eval_finfo and nested_finfo after every time eval step is pushed is unnecessary in ExecInitSubscriptingRef() function. We need them only for EEOP_SBSREF_OLD, EEOP_SBSREF_ASSIGN and EEOP_SBSREF_FETCH steps. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extracting GiST index structure stats?
Hello, On 15.04.2017 20:38, Thomas Mercieca wrote: Hi all, I am interested in getting simple statistics of a GiST index structure. For example, heightof tree. It seems that the other indexes have a metapage for this. I am still unsure but it looks to me like the GiST access method internal does not work exactly in this way so it is a bit more complicated to extract this data. Is this correct? If I wanted to extract this kind of stat (back to the height of tree example) what would be the right approach? Thanks a lot. Have you tried the 'gevel' module? You can find it here: http://www.sai.msu.su/~megera/wiki/Gevel -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgres_fdw: support parameterized foreign joins
On 05.04.2017 12:20, Etsuro Fujita wrote: Rebased. Attached is an updated version created on top of the latest patch "epqpath-for-foreignjoin" [1]. Other changes: * Added a bit more regression tests with FOR UPDATE clause to see if CreateLocalJoinPath works well for parameterized foreign join paths. * Added/revised comments a bit. Thank you! > scan_clauses=NIL for a join relation. So, for a join relation we use > fpinfo->remote_conds and fpinfo->local_conds, instead. (Note that those > conditions are created at path creation time, ie, > postgresGetForeignJoinPaths. See foreign_join_ok.) Oh, I see. I've missed that condition. Marked the patch as "Ready for Commiter". But the patch should be commited only after the patch [1]. 1. https://www.postgresql.org/message-id/424933d7-d6bb-4b8f-4e44-1fea212af083%40lab.ntt.co.jp -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Generic type subscripting
On 05.04.2017 16:06, Arthur Zakirov wrote: I'd like to focus on "refevalfunc" and "refnestedfunc" fields as I did earlier. I think using Oid type for them is a bad approach. "..._fetch" and "..._assign" functions in catalog is unnecessary movement to me. User of subscript of his type may think the same. But he won't see the code and won't know why he needs these functions. And so "..._fetch" and "..._assign" functions in catalog is a bad design to me. But, of course, it is just my opinion. This approach is the main think which we should resolve first, because after commiting the patch it will be hard to fix it. I've read olders messages and thread. I see now that this approach was made with other hackers. I've just been confused when I've been implementing subscript for ltree. Sorry if I confused you. Any opinions about the patch? -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Generic type subscripting
On 05.04.2017 16:06, Arthur Zakirov wrote: On 04.04.2017 15:41, Dmitry Dolgov wrote: Sorry for late reply. Here is a new version of the patch, I rebased it and fixed those issues you've mentioned (pretty nasty problems, thank you for noticing). Thank you! I've looked at the patch again. Sorry maybe it's too naive. Also I was wondering. + element_type_id = transformArrayType(&array_type, &array_typ_mode); + sbsref->refelemtype = element_type_id; I don't understand this part of the patch. Why is it necessary to execute transformArrayType() second time? It was executed in transformContainerSubscripts(). + if (!OidIsValid(elementType)) + elementType = containerType; This part looks strange to me too. It this parts are necessary it would be good to add comments. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Generic type subscripting
On 04.04.2017 15:41, Dmitry Dolgov wrote:
Sorry for late reply. Here is a new version of the patch, I rebased it and
fixed those issues you've mentioned (pretty nasty problems, thank you for
noticing).
Thank you!
I've looked at the patch again.
I'd like to focus on "refevalfunc" and "refnestedfunc" fields as I did
earlier. I think using Oid type for them is a bad approach. "..._fetch"
and "..._assign" functions in catalog is unnecessary movement to me.
User of subscript of his type may think the same. But he won't see the
code and won't know why he needs these functions.
And so "..._fetch" and "..._assign" functions in catalog is a bad design
to me. But, of course, it is just my opinion. This approach is the main
think which we should resolve first, because after commiting the patch
it will be hard to fix it.
static int ArrayCount(const char *str, int *dim, char typdelim);
+bool isAssignmentIndirectionExpr(ExprState *exprstate);
static void ReadArrayStr(char *arrayStr, const char *origStr,
I think isAssignmentIndirectionExpr() here was forgoten to delete,
because isAssignmentIndirectionExpr() is in execExpr.c now.
+ if (subexpr == NULL)
+ {
+ lowerIndexpr = lappend(lowerIndexpr, subexpr);
+ continue;
+ }
+
+
There is the extra line here after the brace.
if (array_type != sbsref->refcontainertype)
{
node = coerce_to_target_type(pstate,
node, array_type,
sbsref->refcontainertype, sbsref->reftypmod,
COERCION_ASSIGNMENT,
COERCE_IMPLICIT_CAST,
-1);
/* can fail if we had int2vector/oidvector, but not for
true domains */
if (node == NULL && node->type != 0)
ereport(ERROR,
(errcode(ERRCODE_CANNOT_COERCE),
errmsg("cannot cast type %s to
%s",
format_type_be(array_type),
format_type_be(sbsref->refcontainertype)),
parser_errposition(pstate,
0)));
PG_RETURN_POINTER(node);
}
Also I was wondering do we need this code in array_subscript_parse()? I
haven't understood the purpose of it. If it is necessary then would be
good to add explain comment.
--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgres_fdw: support parameterized foreign joins
On 23.03.2017 15:45, Etsuro Fujita wrote: Done. Also, I added regression tests and revised code and comments a bit. (As for create_foreignscan_path(), I haven't done anything about that yet.) Please find attached a new version created on top of [1]. Thank you! I didn't notice that it is necessary to apply the patch "epqpath-for-foreignjoin". I have a few comments. * innersortkeys are the sort pathkeys for the inner side of the mergejoin + * req_outer_list is a list of sensible parameterizations for the join rel */ I think it would be better if the comment explained what type is stored in req_outer_list. So the following comment would be good: "req_outer_list is a list of Relids of sensible parameterizations for the join rel" ! Assert(foreignrel->reloptkind == RELOPT_JOINREL); ! Here the new macro IS_JOIN_REL() can be used. ! /* Get the remote and local conditions */ ! remote_conds = list_concat(list_copy(remote_param_join_conds), ! fpinfo->remote_conds); ! local_param_join_exprs = ! get_actual_clauses(local_param_join_conds); ! local_exprs = list_concat(list_copy(local_param_join_exprs), ! fpinfo->local_conds); Is this code correct? 'remote_conds' and 'local_exprs' are initialized above when 'scan_clauses' is separated. Maybe better to use 'remote_conds' and 'local_exprs' instead of 'fpinfo->remote_conds' and 'fpinfo->local_conds' respectively? And the last. The patch needs rebasing because new macroses IS_JOIN_REL() and IS_UPPER_REL() were added. And the patch is applied with errors. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Generic type subscripting
2017-03-28 19:31 GMT+03:00 Dmitry Dolgov <9erthali...@gmail.com>:
> On 28 March 2017 at 12:08, Dmitry Dolgov <9erthali...@gmail.com> wrote:
>>
>> Wow, I didn't notice that, sorry - will fix it shortly.
>
> So, here is the corrected version of the patch.
Thank you!
The patch looks good to me. But it is not applied :) I think it is
because of new FTS functions for jsonb. The patch need rebasing.
But, from my point of view, it would be nice if the code mentioned
earlier was improved:
> + foreach(l, sbsref->reflowerindexpr)
> + {
> + List *expr_ai = (List *) lfirst(l);
> + A_Indices *ai = (A_Indices *) lfirst(list_tail(expr_ai));
> +
> + subexpr = (Node *) lfirst(list_head(expr_ai));
It is necessary for arrays of course because of logic mentioned in the
documentation.
> If any dimension is written as a slice, i.e., contains a colon, then all
> dimensions are treated as slices. Any dimension that has only a single number
> (no colon) is treated as being from 1 to the number specified.
But it would be better if SubscriptingRef structure had a new field of
List type. This field can store list of booleans, which means is there
slices or not. I think it can improve readability of the code.
--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgres_fdw: support parameterized foreign joins
Hello, On 31.03.2017 18:47, David Steele wrote: Hi Arthur. On 3/23/17 8:45 AM, Etsuro Fujita wrote: Done. Also, I added regression tests and revised code and comments a bit. (As for create_foreignscan_path(), I haven't done anything about that yet.) Please find attached a new version created on top of [1]. Are you planning to review this patch? Thanks, Yes, I wanted to review the patch. But there was a lack of time this week. I marked myself as reviewer in the commitfest app. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Generic type subscripting
2017-03-31 5:32 GMT+03:00 Dmitry Dolgov <9erthali...@gmail.com>: > On 30 March 2017 at 19:36, Arthur Zakirov wrote: >> >> The last point is about the tutorial. As Tom pointed it is not useful when >> the tutorial doesn't execute. It happens because there is not "custom" type >> in subscripting.sql. > > I'm confused. Maybe I'm missing something, but there is "custom" type in > this file: Sorry for confusing. I should have been more careful. I've mixed up NOTICE message with error message and I haven't noticed CREATE TYPE command. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Generic type subscripting
On 29.03.2017 20:14, Arthur Zakirov wrote:
I wanted to implement subscripting for ltree or hstore extensions.
Subscripting for ltree looks more interesting. Especially with slicing.
But I haven't done it yet. I hope that I will implement it tomorrow.
ltree
-
I've implemented fetching ltree elements using subscripting. But haven't
implemented assigning ltree elements yet. I'll send second patch after
implementing assigning.
Now you can execute the following query:
SELECT ('Top.Science.Astronomy.Astrophysics'::ltree)[1:2];
ltree
-
Top.Science
Comments
But I've noticed about some points.
In array_subscript_parse() passed uninitialized values of "typesource"
and "typeneeded" variables for coerce_to_target_type().
+ typesource = exprType(assignExpr);
+ typesource = is_slice ? sbsref->refcontainertype :
sbsref->refelemtype;
Here is the bug. Second variable should be "typeneeded". Moreover these
assignments should be moved up to first coerce_to_target_type() execution.
+ foreach(l, sbsref->reflowerindexpr)
+ {
+ List *expr_ai = (List *) lfirst(l);
+ A_Indices *ai = (A_Indices *) lfirst(list_tail(expr_ai));
+
+ subexpr = (Node *) lfirst(list_head(expr_ai));
This code looks like a magic. This happens because of appending
A_Indeces to lowerIndexpr:
- subexpr = NULL;
+ lowerIndexpr = lappend(lowerIndexpr,
list_make2(subexpr, ai));
}
And this A_Indeces used only when slicing is not used to make a constant
1. Maybe there are another way?
Also it would be better if "refevalfunc" and "refnestedfunc" had
pointers to functions not Oid type. Now you need to create "..._fetch"
and "..._assign" functions in catalog and in "..._parse" function you
need get their Oid using to_regproc() function.
Can we use IndexAmRoutine structure method, when you use only pointers
to necessary functions? You can see an example in blhandler() function
in blutils.c.
The last point is about the tutorial. As Tom pointed it is not useful
when the tutorial doesn't execute. It happens because there is not
"custom" type in subscripting.sql. Also it contradicts the README of
tutorials.
--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Generic type subscripting
On 28.03.2017 19:31, Dmitry Dolgov wrote:
On 28 March 2017 at 12:08, Dmitry Dolgov <9erthali...@gmail.com
<mailto:9erthali...@gmail.com>> wrote:
Wow, I didn't notice that, sorry - will fix it shortly.
So, here is the corrected version of the patch.
I have some picky comments.
I'm not sure that "typsbsparse" is better than "typsubscripting" or
"typsubparse". Maybe "typsubsparse"?
+ typsubscripting
+ regproc
Here you didn't fix "typsubscripting" to new name.
+ JSON subscripting
+
+ JSONB data type support array-style subscripting expressions to extract or
update particular element. An example of subscripting syntax:
Should be "JSONB data type supports".
+ to handle subscripting expressions. It should contains logic for verification
+ and decide which function must be used for evaluation of this expression.
+ For instance:
Should be "It should contain".
+
+ JSON subscripting
+
+ JSONB data type support array-style subscripting expressions to extract or
update particular element. An example of subscripting syntax:
You have implemented jsonb subscripting. The documentation should be
fixed to:
+
+ jsonb Subscripting
+
+ jsonb data type support array-style subscripting
expressions to extract or update particular element. An example of
subscripting syntax:
I think IsOneOf() macros should be removed. Since it is not used anywhere.
+ Assert(subexpr != NULL);
+
+ if (subexpr == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+errmsg("jsonb subscript does not support
slices"),
+parser_errposition(pstate,
exprLocation(
+ ((Node *)
lfirst(sbsref->refupperindexpr->head));
Here one of the conditions is excess. Better to delete assert condition
I think.
I've tried tests from message [1]. They looks good. Performance looks
similar for subscripting without patch and with patch.
I wanted to implement subscripting for ltree or hstore extensions.
Subscripting for ltree looks more interesting. Especially with slicing.
But I haven't done it yet. I hope that I will implement it tomorrow.
1.
https://www.postgresql.org/message-id/CAKNkYnz_WWkzzxyFx934N%3DEp47CAFju-Rk-sGeZo0ui8QdrGmw%40mail.gmail.com
--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Generic type subscripting
Hello, On 27.03.2017 23:28, Dmitry Dolgov wrote: Here is a new version of this patch. What was changed: * I rebased code against the latest version of master and adapted recent changes about the expression execution * Several names (functions and related pg_type column) were changed * A new oid function field was introduced to handle nested assignment situation * I updated the documentation for patch * `MAXDIM` was replaced by `MAX_SUBSCRIPT_DEPTH` * I returned one `SubscriptingRef` for both fetch & assign operations, since there is no real readability improvements at this point (they're already separated at the time of evaluation, and besides the evaluation code fetch & assign are handled almost identically). Your patch reverts commits from 25-26 march. And therefore contains 15000 lines. I think the patch needs rebasing. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Generic type subscripting
On 24.03.2017 18:29, Tom Lane wrote: David Steele writes: Do you have an idea when you will have a patch ready? We are now into the last week of the commitfest. I see one question for Tom, but it's not clear that this would prevent you from producing a new patch. FWIW, I'm up to my eyeballs in Andres' faster-expressions patch, and won't have time to think about this one for at least a couple more days. regards, tom lane I can try to review a new version of the patch, when Dmitry will send it. If no one objects. Besides, I've seen the previous versions of the patch from previous commitfest. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present
Hello, On 19.03.2017 21:45, Michael Banck wrote: So the patch I sent earlier creates the slot in ReceiveXlogStream() in receivewal.c, as that's where the temp slot gets created as well, but now I wonder whether that is maybe not the best place, as pg_receivewal also calls that function. The other problem with receivewal.c is that `verbose' isn't around in it so I don't how I'd print out a message there. So probably it is better to create the slot in pg_basebackup.c's StartLogStreamer(), see the attached first patch, that one also adds a verbose message. I think such too. I suppose it is more clearly. StartLogStreamer() is better place for creating permanent and temporary slots. Also maybe it would be good if pg_basebackup had a way to drop created slot. Although "drop slot" is not related with concept of automatically created slots, it will good if user will have a way to drop slots. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgres_fdw: support parameterized foreign joins
Hello, 2017-02-27 12:40 GMT+03:00 Etsuro Fujita : > Hi, > > I'd like to propose to support parameterized foreign joins. Attached is a > patch for that, which has been created on top of [1]. > Can you rebase the patch? It is not applied now. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IF NOT EXISTS option for CREATE SERVER and CREATE USER MAPPING statements
2017-03-14 15:55 GMT+03:00 Ashutosh Bapat : > > I noticed that the earlier error message was using "server" instead of > "foreign server", while the new message uses the later one. Usually, > when converting an error to notice, we don't expect such changes. But > many other error messages are using "foreign server" instead of > "server", so probably this one needed a change anyway. But then, the > command to create a foreign server is not "CREATE FOREIGN SERVER", > it's "CREATE SERVER", so users are already getting confused? Actually, there are other messages with "server". For example, in the AlterForeignServerOwner() or in the postgres_fdw code. Maybe it is better to not change "server" to "foreign server" in "create_foreign_server_if_not_exists.patch"? I think it will be better to fix all such messages with a separate patch, If we will decide that it is necessary to change "server" messages. > > I don't see similar change in the error message for the user mapping. > Do we need to change "server" to "foreign server" in case of user > mapping? The doc changes didn't compile with error > "osx:ref/create_user_mapping.sgml:52:15:E: document type does not > allow element "VARLISTENTRY" here; assuming missing "VARIABLELIST" Indeed! Missed that. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
