Re: PQHost() undefined behavior if connecting string contains both host and hostaddr types
On Sun, Jan 14, 2018 at 02:19:26PM +1100, Haribabu Kommi wrote: > While working on [1], we find out the inconsistency in PQHost() behavior > if the connecting string that is passed to connect to the server contains > multiple hosts with both host and hostaddr types. For example, > > host=host1,host2 hostaddr=127.0.0.1,127.0.0.1 port=5434,5432 > > As the hostaddr is given preference when both host and hostaddr is > specified, so the connection type for both addresses of the above > conninfo is CHT_HOST_ADDRESS. So the PQhost() returns the > conn->pghost value i.e "host1,host2" instead of the actual host that > is connected. During the discussion of adding the client-side connection parameters to pg_stat_wal_receiver, which is useful when the client specifies multiple hosts and ports, it has been discussed that introducing a new API in libpq to get effective host, hostaddr and port values would be necessary in order to get all the useful information wanted, however this has a larger impact than initially thought as any user showing the host information in psql's PROMPT would be equally confused. Any caller of PQhost have the same problem. > Instead of checking the connection type while returning the host > details, it should check whether the host is NULL or not? with this > change it returns the expected value for all the connection types. I mentioned that on the other thread, but this seems like an improvement to me, which leads to less confusion. See here for more details regarding what we get today on HEAD: https://www.postgresql.org/message-id/20180109011547.GE76418%40paquier.xyz -- Michael signature.asc Description: PGP signature
Re: [HACKERS] UPDATE of partition key
On 13 January 2018 at 02:56, Robert Haas wrote: > On Fri, Jan 12, 2018 at 12:23 PM, Amit Khandekar > wrote: >>> (1) if they need it by subplan index, first use >>> subplan_partition_offsets to convert it to a per-leaf index >> >> Before that, we need to check if there *is* an offset array. If there >> are no partitions, there is only going to be a per-subplan array, >> there won't be an offsets array. But I guess, you are saying : "do the >> on-demand allocation only for leaf partitions; if there are no >> partitions, the per-subplan maps will always be allocated for each of >> the subplans from the beginning" . So if there is no offset array, >> just return mtstate->mt_per_subplan_tupconv_maps[subplan_index] >> without any further checks. > > Oops. I forgot that there might not be partitions. I was assuming > that mt_per_subplan_tupconv_maps wouldn't exist at all, and we'd > always use subplan_partition_offsets. Both that won't work in the > inheritance case. > >> But after that, I am not sure then why is mt_per_sub_plan_maps[] array >> needed ? We are always going to convert the subplan index into leaf >> index, so per-subplan map array will not come into picture. Or are you >> saying, it will be allocated and used only when there are no >> partitions ? From one of your earlier replies, you did mention about >> trying to share the maps between the two arrays, that means you were >> considering both arrays being used at the same time. > > We'd use them both at the same time if we didn't have, or didn't use, > subplan_partition_offsets, but if we have subplan_partition_offsets > and can use it then we don't need mt_per_sub_plan_maps. > > I guess I'm inclined to keep mt_per_sub_plan_maps for the case where > there are no partitions, but not use it when partitions are present. > What do you think about that? Even where partitions are present, in the usual case where there are no transition tables we won't require per-leaf map at all [1]. So I think we should keep mt_per_sub_plan_maps only for the case where per-leaf map is not allocated. And we will not allocate mt_per_sub_plan_maps when mt_per_leaf_maps is needed. In other words, exactly one of the two maps will be allocated. This is turning out to be close to what's already there in the last patch versions: use a single map array, and an offsets array. The difference is : in the patch I am using the *same* variable for the two maps. Where as, now we are talking about two different array variables for maps, but only allocating one of them. Are you ok with this ? I think the thing you were against was to have a common *variable* for two purposes. But above, I am saying we have two variables but assign a map array to only *one* of them and leave the other unused. - Regarding the on-demand map allocation Where mt_per_sub_plan_maps is allocated, we won't have the on-demand allocation: all the maps will be allocated initially. The reason is becaues the map_is_required array is only per-leaf. Or else, again, we need to keep another map_is_required array for per-subplan. May be we can support the on-demand stuff for subplan maps also, but only as a separate change after we are done with update-partition-key. - Regarding mt_per_leaf_tupconv_required, I am thinking we can make it a bool array, and name it : mt_per_leaf_map_not_required. When it is true for a given index, it means, we have already called convert_tuples_by_name() and it returned NULL; i.e. it means we are sure that map is not required. A false value means we need to call convert_tuples_by_name() if it is NULL, and then set mt_per_leaf_map_not_required to (map == NULL). Instead of a bool array, we can even make it a Bitmapset. But I think access would become slower as compared to array, particularly because it is going to be a heavily used function. - [1] - For update-tuple-routing, only per-subplan access is required; - For transition tables, per-subplan access is required, and additionally per-leaf access is required when tuples are update-routed - So if both update-tuple-routing and transition tables are required, both of the maps are needed. -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company
Re: WIP: a way forward on bootstrap data
I'm 1000% on board with replacing oid constants with symbolic names that get substituted programmatically. However I wonder why we're bothering inventing a new syntax that doesn't actually do much more than present static tabular data. If things like magic proname->prosrc behaviour are not valuable then we're not getting much out of this perl-friendly syntax that a simpler more standard format wouldn't get us. So just as a straw man proposal What if we just replaced the data file with a csv file that could be maintained in a spreadsheet. It could easily be parsed by perl and we could even have perl scripts that load the records into memory and modify them. You could even imagine writing a postgres script that loaded the csv file into a temporary table, did complex SQL updates or other DML, then wrote it back out to a csv file.
Re: [HACKERS] [PATCH] Improve geometric types
> I found seemingly inconsistent handling of NaN.
>
> - Old box_same assumed NaN != NaN, but new assumes NaN ==
> NaN. I'm not sure how the difference is significant out of the
> context of sorting, though...
There is no box != box operator. If there was one, previous code
would return false for every input including NaN, because it was
ignorant about NaNs other than a few bandaids to avoid crashes.
My idea is to make the equality (same) operators behave like the float
datatypes treating NaNs as equals. The float datatypes also treat
NaNs as greater than any non-NAN. We don't need to worry about this
part now, because there are no basic comparison operators defined for
the geometric types.
> - box_overlap()'s behavior has not been changed. As the result
> box_same and box_overlap now behaves differently concerning
> NaN handling.
After your previous email, I though this would be the right thing to
do. I made the containment and placement operators return false for
NaN input unless we can find the result using non-NaN coordinates. Do
you find this behaviour reasonable?
> - line_construct_pts has been changed so that generates
> {NaN,-1,NaN} for two identical points (old code generated
> {-1,0,0}) but I think the right behavior here is erroring out.
> (as line_in behaves.)
I agree. We should also check for the endpoints being the same on
lseg_interpt functions. I will make the changes on the next version.
> I feel that it'd better to define how to handle non-numbers
> before refactoring. I prefer to just denying NaN as a part of
> the geometric types, or any input containing NaN just yields a
> result filled with NaNs.
It would be nice to deny NaNs altogether, but I don't think it is
feasible. People cannot restore their existing data if we start doing
that. It would also require us to check any result we emit being NaN
and error out in more cases.
> The next point is reasonability of the algorithm and consistency
> among related functions.
>
> - line_parallel considers the EPSILON(ugaa) with different scale
> from the old code, but both don't seem reasonable.. It might
> not be the time to touch it, but if we changed the behavior,
> I'd like to choose reasonable one. At least the tolerance of
> parallelity should be taken by rotation, not by slope.
I think you are right. Vector based algorithms would be good for many
other operations as well. This would be more changes, though. I am
try to reduce the amount of changes to increase my chances to get this
committed. I just used slope() there to increase the code reuse and
to make the operation symmetrical. I can revert it back to its
previous state, if you thing it is better.
> So we might should calculate the distance in different (not
> purely mathematical/geometrical) way. For example, if we can
> assume the geometric objects are placed mainly around the origin,
> we could take the distance between the points nearest to origin
> on both lines. (In other words, between the foots of
> perpendicular lines from the origin onto the both lines). Of
> couse this is not ideal but...
The EPSILON is unfair to coordinates closer to the origin. I am not
sure what it the best way to improve this. I am trying to avoid
dealing with EPSILON related issues in the scope of these changes.
> At last, just a simple comment.
>
> - point_eq_point() assumes that NaN == NaN. This is an inherited
> behavior from old float[48]_cmp_internal() but it's not a
> common treat. point_eq_point() needs any comment about the
> special definition as float[48]_cmp_internal has.
I hope I answered this on the previous questions. I will incorporate
the decision to threat NaNs as equals into the comments and the commit
messages on the next version, if we agree on it.
Re: [PATCH] Find additional connection service files in pg_service.conf.d directory
On Fri, Jan 12, 2018 at 09:53:51AM -0500, Curt Tilmes wrote: > This patch adds an extra search for service connection definitions if > the current places fail > to find the service (~/.pg_service.conf /pg_service.conf). > It will then search > every readable file in the directory /pg_service.conf.d. > > What do you think? If you are looking for feedback regarding your patch, please be sure to register it to the next commit fest: https://commitfest.postgresql.org/17/ Here are as well some guidelines about patch submission and review process: https://wiki.postgresql.org/wiki/Submitting_a_Patch -- Michael signature.asc Description: PGP signature
Re: proposal: alternative psql commands quit and exit
On Sun, Jan 14, 2018 at 2:38 AM, Chapman Flack wrote: > On 01/13/18 21:36, Everaldo Canuto wrote: > > > I don't wanna be irritating but here every postgres user I found really > > can't understand how the original patch was not accepted, people against > it > > did not show good reasons. > > As I was watching this thread go by, the reasons being shown seemed good > to me, and they were being raised by some of the people most responsible > (as one quickly learns by scanning old archives), over the years, for > postgres being the excellent thing that it is, so it would not be my > first instinct to dismiss them. > Yes, your are right and I Postgres is the best database in my opinion and it tells me that community here must be responsible. I just think that Postgres could be a little more friendly, it is really hard to me to see my customers choosing other databases because they feel Postgres is hard, I end up loosing amazing Postgres features. Unfortunately you are right, my first instinct was to dismiss them and it could be a problem on my side but as I said previously on this thread, it is really frustrating for me to see a so small patch that "fix" one of new comers problem (also casual users and user of multiple databases) and don't hurts anything in PG end up on a directions that makes the problem even worst. Also I don't see a consensus on this thread and I don't understand how decisions are taken. Again, maybe the problem is with me but is a fact that people feel like Postgres is hard. Since it is my last message to this thread (I don't think my words will convince anyone) let me explain how came here: I was attending a conference with a friend who is also the owner of a Postgres support company and we start to discuss some small problems that could be relatively easy to fix and could make Postgres even better. At some point he just point why a experienced C developer like me (we would love to develop in C) don't contribute to the project and we came up with a list of things to improve and I decide to make a test with something simple and fast to implement to test if Postgres is not like a GNOME community. So, here I am. > One of the things I have come to really appreciate about this community > (in which I am still a relative newcomer) is that when disagreements arise, > you can see them being worked through by consideration of technical points > and somewhat measurable principles. In that light, if there's a better > conclusion you think is worth advocating, that would fit right in if done > in ways that respond to, and address in some way, the points already > raised; that adds more to the conversation than just saying they were > not "good". > > -Chap > You are right again and could be something wrong with my side, I really can't see the arguments agains as good, they look me like a personal taste and not technical. I through that even my arguments or people that agree with this patch is also more a personal opinion than technical but I think it will not hurt if we made Postgres more friendly to newcomers. What is really funny is that I was prepared for long and hard discussions on future because but not on this. On my list of next patches was: * SHOW TABLES * DES[CRIBE] * SEARCH path environment var for \i and \ir (it is really useful) * Changes on pg_hba default values so local sql clients can connect without change it * and so on... Anyway, sorry if I offended someone, It is not my intention. Keep the good work guys and even if PG is the best, maybe take some look at MariaDB will not hurt too much ;-) Regards.
Re: WIP: a way forward on bootstrap data
Greg Stark writes: > I'm 1000% on board with replacing oid constants with symbolic names > that get substituted programmatically. Yeah, that's almost an independent feature --- we could do that without any of this other stuff, if we wanted. > However I wonder why we're bothering inventing a new syntax that > doesn't actually do much more than present static tabular data. If > things like magic proname->prosrc behaviour are not valuable then > we're not getting much out of this perl-friendly syntax that a simpler > more standard format wouldn't get us. TBH, the thing that was really drawing my ire about that was that John was inventing random special rules and documenting them *noplace* except for the guts of some perl code. If I have to read perl code to find out what the catalog data means, I'm going to be bitching loudly. That could be done better --- one obvious idea is to add a comment to the relevant .h file, next to the field whose value will be implicitly calculated. > So just as a straw man proposal What if we just replaced the data > file with a csv file that could be maintained in a spreadsheet. Bleah --- that's no better than what we have today, just different. And "maintained in a spreadsheet" doesn't sound attractive to me; you'd almost certainly lose comments, for instance. regards, tom lane
Re: WIP: a way forward on bootstrap data
I wrote: > Another thing that I'd sort of hoped might happen from this patchset > is to cure the problem of keeping some catalog headers safe for > client-side inclusion, because some clients want the OID value macros > and/or macros for column values (eg PROVOLATILE_IMMUTABLE), so they > currently have to #include those headers or else hard-code the values. > We've worked around that to date with ad-hoc solutions like splitting > function declarations out to pg_*_fn.h files, but I never liked that > much. With the OID value macros being moved out to separate generated > file(s), there's now a possibility that we could fix this once and for all > by making client-side code include those file(s) not pg_type.h et al > themselves. But we'd need a way to put the column-value macros into > those files too; maybe that's too messy to make it practical. I had a thought about how to do that. It's clearly desirable that that sort of material remain in the manually-maintained pg_*.h files, because that's basically where you look to find out C-level details of what's in a particular catalog. However, that doesn't mean that that's where the compiler has to find it. Imagine that we write such sections of the catalog .h files like #ifdef EXPOSE_TO_CLIENT_CODE /* * ... comment here ... */ #define PROVOLATILE_IMMUTABLE 'i' /* never changes for given input */ #define PROVOLATILE_STABLE 's' /* does not change within a scan */ #define PROVOLATILE_VOLATILE'v' /* can change even within a scan */ #endif /* EXPOSE_TO_CLIENT_CODE */ Like CATALOG_VARLEN, the symbol EXPOSE_TO_CLIENT_CODE is never actually defined to the compiler. What it does is to instruct genbki.pl to copy the material up to the matching #endif into the generated file for this catalog. So, for each catalog header pg_foo.h, there would be a generated file, say pg_foo_d.h, containing: * Natts_ and Anum_ macros for pg_foo * Any EXPOSE_TO_CLIENT_CODE sections copied from pg_foo.h * Any OID-value macros for entries in that catalog pg_foo.h would contain a #include for pg_foo_d.h, so that backend-side code would obtain all these values the same as it did before. But the new policy for client code would be to include pg_foo_d.h *not* pg_foo.h, and so we are freed of any worry about whether pg_foo.h has to be clean for clients to include. We could re-merge the various pg_foo_fn.h files back into the main files, if we wanted. The contents of EXPOSE_TO_CLIENT_CODE sections wouldn't necessarily have to be just macros --- they could be anything that's safe and useful for client code. But that's probably the main usage. regards, tom lane
Re: WIP: a way forward on bootstrap data
On 1/14/18, Tom Lane wrote: > I'm not sure about the decision to move all the OID macros into > one file; that seems like namespace pollution. > The design I'd kind of imagined was one generated file of #define's > per pg_*.h file, not just one giant one. First, thanks for the comments. I will start incorporating them in a few days to give others the chance to offer theirs. I'm inclined to agree about namespace pollution. One stumbling block is the makefile changes to allow OID symbols to be visible to non-backend code. Assuming I took the correct approach for the single file case, adapting that to multiple files would require some rethinking. > It's especially > odd that you did that but did not consolidate fmgroids.h with > the macros for symbols from other catalogs. Point taken. > It would be really nice, also, if the attribute number macros > (Natts_pg_proc, Anum_pg_proc_proname, etc) could be autogenerated. > Manually renumbering those is one of the bigger pains in the rear > when adding catalog columns. It was less of a pain than adjusting > the DATA lines of course, so I never figured it was worth doing > something about in isolation --- but with this infrastructure in > place, that's manual work we shouldn't have to do anymore. Searching the archives for discussion of Anum_* constants [1], I prefer your one-time suggestion to use enums instead. I'd do that, and then have Catalog.pm throw an error if Natts_* doesn't match the number of columns. That's outside the scope of this patch, however. > Another thing that I'd sort of hoped might happen from this patchset > is to cure the problem of keeping some catalog headers safe for > client-side inclusion, because some clients want the OID value macros > and/or macros for column values (eg PROVOLATILE_IMMUTABLE), so they > currently have to #include those headers or else hard-code the values. > We've worked around that to date with ad-hoc solutions like splitting > function declarations out to pg_*_fn.h files, but I never liked that > much. With the OID value macros being moved out to separate generated > file(s), there's now a possibility that we could fix this once and for all > by making client-side code include those file(s) not pg_type.h et al > themselves. But we'd need a way to put the column-value macros into > those files too; maybe that's too messy to make it practical. To make sure I understand you correctly: Currently we have pg_type.h oid symbols, column symbols pg_type_fn.h function decls And assuming we go with one generated oid symbol file per header, my patch would end up with something like pg_type.h column symbols (#includes pg_type_sym.h) pg_type_fn.h function decls pg_type_sym.h oid symbols (generated) And you're saying you'd prefer pg_type.h function decls (#includes pg_type_sym.h) pg_type_sym.h oid symbols, column symbols (generated) I agree that it'd be messy to drive the generation of the column symbols. I'll think about it. What about pg_type.h function decls (#includes pg_type_sym.h) pg_type_sym.h column symbols (static, #includes pg_type_oids.h) pg_type_oids.h oid symbols (generated) It's complicated, but arguably no more so than finding someplace more distant to stick the column symbols and writing code to copy them. It'd be about than 20 *_sym.h headers and 10 *_oids.h headers. > The .dat files need to have header comments that follow project > conventions, in particular they need to contain copyright statements. > Likewise for generated files. Okay. > I've got zero faith that the .h files will hold still long enough > for these patches to be reviewed and applied. The ones that touch > significant amounts of data need to be explained as "run this script > on the current data", rather than presented as static diffs. I've already rebased over a catalog change and it was not much fun, so I'd be happy to do it this way. > I'm not really thrilled by the single-purpose "magic" behaviors added > in 0007, such as computing prosrc from proname. I think those will > add more confusion than they're worth. Okay. I still think generating pg_type OID symbols is worth doing, but I no longer think this is a good place to do it. > In 0010, you relabel the types of some OID columns so that genbki.pl > will know which lookup to apply to them. That's not such a problem for > the relabelings that are just macros and genbki.pl converts back to > type OID in the .bki file. But you also did things like s/Oid/regtype/, > and that IS a problem because it will affect what client code sees in > those catalog columns. We've discussed changing those columns to > regfoo types in the past, and decided not to, because of the likelihood > of breaking client queries. I do not think this patch gets to change > that policy. So the way to identify the lookup rule needs to be > independent of whether the column is declared as Oid or an Oid alias type. > Perhaps an explicit marker telli
Re: Logical decoding fast-forward and slot advance
On 08/01/18 08:02, Simon Riggs wrote:
> On 31 December 2017 at 10:44, Petr Jelinek
> wrote:
>
>> Attached is patch which adds ability to do fast-forwarding while
>> decoding. That means wal is consumed as fast as possible and changes are
>> not given to output plugin for sending. The implementation is less
>> invasive than I originally though it would be. Most of it is just
>> additional filter condition in places where we would normally filter out
>> changes because we don't yet have full snapshot.
>
> Looks good.
>
Thanks.
> The precise definition of "slot advance" or "fast forward" isn't
> documented in the patch. If we advance past everything, why is there
> not just one test in LogicalDecodingProcessRecord() to say if
> (ctx->fast_forward)? Why put it in multiple decoding subroutines?
>
Because we still need to track transactions (otherwise slot's restart
position will not move forward) and mark transactions which did DDL
changes so that historical snapshots are made. Otherwise if we moved
slot forward and then started real decoding from that position we'd have
wrong view of catalogs.
We'd have to write different version of LogicalDecodingProcessRecord()
and duplicate some of the code in the Decode* functions there which
seems like it would be harder to maintain. I might be inclined to do it
with this approach if the current approach would mean adding new branch
into every Decode* function, but since there is already branch for
filtering actions during initial snapshot build, I think it's better to
just extend that.
> If ctx->fast_forward is set it might throw off other opps, so it would
> be useful to see some Asserts elsewhere to make sure we understand and
> avoid breakage
Hmm, I think the really only places where this can be issue and also can
be checked using Assert are the cb wrappers in logical.c which call the
output plugin (output plugin is not supposed to be called when
fast-forwarding) so I Added assert to each of them.
> In pg_replication_slot_advance() the moveto variable is set to
> PG_GETARG_LSN(1) and then unconditionally overwritten before it is
> used for anything. Why?
>
Eh, there is missing Min, it should be used for clamping, not done
unconditionally. Fixed and added regression test for this.
Updated version attached.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
From 00b7a3e4abcba36e326dcb4d0c388d6e43c2 Mon Sep 17 00:00:00 2001
From: Petr Jelinek
Date: Fri, 29 Dec 2017 09:46:35 +0100
Subject: [PATCH] Add pg_replication_slot_advance() function
Advances replication slot to specified position. Works both with logical
and physical slots.
This also adds fast forward mode for logical decoding which is used by
the pg_replication_slot_advance() function as well as slot creation.
---
contrib/test_decoding/expected/slot.out| 30
contrib/test_decoding/sql/slot.sql | 15 ++
doc/src/sgml/func.sgml | 19 +++
src/backend/replication/logical/decode.c | 41 +++--
src/backend/replication/logical/logical.c | 30 +++-
src/backend/replication/logical/logicalfuncs.c | 1 +
src/backend/replication/slotfuncs.c| 200 +
src/backend/replication/walsender.c| 1 +
src/include/catalog/pg_proc.h | 2 +
src/include/replication/logical.h | 8 +
10 files changed, 330 insertions(+), 17 deletions(-)
diff --git a/contrib/test_decoding/expected/slot.out b/contrib/test_decoding/expected/slot.out
index 9f5f8a9b76..21e9d56f73 100644
--- a/contrib/test_decoding/expected/slot.out
+++ b/contrib/test_decoding/expected/slot.out
@@ -92,6 +92,36 @@ SELECT data FROM pg_logical_slot_get_changes('regression_slot2', NULL, NULL, 'in
COMMIT
(3 rows)
+INSERT INTO replication_example(somedata, text) VALUES (1, 4);
+INSERT INTO replication_example(somedata, text) VALUES (1, 5);
+SELECT pg_current_wal_lsn() AS wal_lsn \gset
+INSERT INTO replication_example(somedata, text) VALUES (1, 6);
+SELECT end_lsn FROM pg_replication_slot_advance('regression_slot1', :'wal_lsn') \gset
+SELECT slot_name FROM pg_replication_slot_advance('regression_slot2', pg_current_wal_lsn());
+slot_name
+--
+ regression_slot2
+(1 row)
+
+SELECT :'wal_lsn' = :'end_lsn';
+ ?column?
+--
+ t
+(1 row)
+
+SELECT data FROM pg_logical_slot_get_changes('regression_slot1', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
+ data
+-
+ BEGIN
+ table public.replication_example: INSERT: id[integer]:6 somedata[integer]:1 text[character varying]:'6'
+ COMMIT
+(3 rows)
+
+SELECT data FROM pg_logical_slot_get_changes('regression_slot2', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts'
Re: PATCH: psql tab completion for SELECT
And here's a patch to add savepoint protection for tab completion. It could definitely use some scrutiny to make sure it's not messing up the user's transaction. I added some error checking for the savepoint creation and the rollback, and then wrapped it in #ifdef NOT_USED (just as the query error checking is) as I wasn't sure how useful it is for normal use. But I do feel that if psql unexpectedly messes up the transaction, it should report it somehow. How can we tell that we've messed it up for the user? Cheers, Edmund psql-tab-completion-savepoint-v1.patch Description: Binary data
Re: PATCH: psql tab completion for SELECT
On January 14, 2018 5:12:37 PM PST, Edmund Horner wrote: >And here's a patch to add savepoint protection for tab completion. It'd be good to explain what that means, so people don't have to read the patch to be able to discuss whether this is a good idea. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [HACKERS] Adding column_constraint description in ALTER TABLE synopsis
Greetings Lætitia, Amit, * Lætitia Avrot (laetitia.av...@gmail.com) wrote: > Thanks Stephen for the suggestion. I wan't thinking globally enough. I was > planning to look at it today but Amit was faster. So thanks Amit too! This seems to have gotten lost in the shuffle, but Amit's patch still applies cleanly and it looks like a good patch to me, so I've added it to the current CF and marked it as Needs Review. There's been some further discussion of this change over in this thread: https://www.postgresql.org/message-id/flat/20171220090816.25744.72592%40wrigleys.postgresql.org#20171220090816.25744.72...@wrigleys.postgresql.org which seemed to reach the conclusion that the proposed patch makes sense. If someone else would like to review it, that'd be great, otherwise I'll probably get it committed soon. Thanks! Stephen signature.asc Description: PGP signature
Re: [HACKERS] Refactoring identifier checks to consistently use strcmp
On Fri, Jan 12, 2018 at 11:35:48PM +0100, Daniel Gustafsson wrote: > On 11 Jan 2018, at 09:01, Michael Paquier wrote: >> I would like to think that a special section >> dedicated to option compatibility for each command would be welcome to >> track which grammar is supported and which grammar is not supported. > > I’m not sure I follow? > >>> One open question from this excercise is how to write a good test for >>> this. It can either be made part of the already existing test queries >>> or a separate suite. I’m leaning on the latter simply because the >>> case-flipping existing tests seems like something that can be cleaned >>> up years from now accidentally because it looks odd. >> >> Adding them into src/test/regress/ sounds like a good plan to me. > > If there is interest in this patch now that the list exists and aids review, I > can turn the list into a proper test that makes a little more sense than the > current list which is rather aimed at helping reviewers. Sorry if my words were hard to catch here. What I mean here is to add in each command's test file the set of commands which check the compatibility. There is no need to test all the options in my opinion, as just testing one option is enoughto show the purpose. So for example to cover the grounds of DefineAggregate(), you could add a set of commands in create_aggregate.sql. For DefineCollation(), those can go in collate.sql, etc. >> Here is another idea: nuking isstrict and iscachable from CREATE >> FUNCTION syntax and forget about them. I would be tempted of the opinion >> to do that before the rest. > > Thats certainly an option, I have no idea about the prevalence in real life > production environments to have much an opinion to offer. Please let me raise a new thread about this point with a proper patch. That's rather separate to the work you are doing here, even if those parameters are using pg_strcasecmp(). -- Michael signature.asc Description: PGP signature
Re: PATCH: psql tab completion for SELECT
On 15 January 2018 at 14:20, Andres Freund wrote: > On January 14, 2018 5:12:37 PM PST, Edmund Horner wrote: >>And here's a patch to add savepoint protection for tab completion. > > It'd be good to explain what that means, so people don't have to read the > patch to be able to discuss whether this is a good idea. Good idea. In psql if you have readline support and press TAB, psql will often run a DB query to get a list of possible completions to type on your current command line. It uses the current DB connection for this, which means that if the tab completion query fails (e.g. because psql is querying catalog objects that doesn't exist in your server), the current transaction (if any) fails. An example of this happening is: $ psql -h old_database_server psql (10.1, server 9.2.24) Type "help" for help. postgres=# begin; BEGIN postgres=# create table foo (id int); CREATE TABLE postgres=# alter subscription (no tab completions because the pg_subscription table doesn't exist on 9.2! User realises their mistake and types a different command) postgres=# select * from foo; ERROR: current transaction is aborted, commands ignored until end of transaction block This patch: - checks that the server supports savepoints (version 8.0 and later) and that the user is currently idle in a transaction - if so, creates a savepoint just before running a tab-completion query - rolls back to that savepoint just after running the query The result is that on an 8.0 or later server, the user's transaction is still ok: $ psql -h old_database_server psql (11devel, server 9.2.24) Type "help" for help. postgres=# begin; BEGIN postgres=# create table foo (id int); CREATE TABLE postgres=# alter subscription (again, no tab completions) postgres=# select * from p; id (0 rows) postgres=# commit; COMMIT Note that only the automatic tab-completion query is protected; the user can still fail their transaction by typing an invalid command like ALTER SUBSCRIPTION ;.
Removing WITH clause support in CREATE FUNCTION, for isCachable and isStrict
Hi all,
As noticed by Daniel here:
https://www.postgresql.org/message-id/d5f34c9d-3ab7-4419-af2e-12f67581d...@yesql.se
Using a WITH clause takes precendence over what is defined in the main
function definition when using isStrict and isCachable. For example,
when using VOLATILE and IMMUTABLE, an error is raised:
=# create function int42(cstring) returns int42 AS 'int4in'
language internal strict immutable volatile;
ERROR: 42601: conflicting or redundant options
LINE 2: language internal strict immutable volatile;
However when using for example STABLE/VOLATILE in combination with a
WITH clause, then things get prioritized, and in this case the WITH
clause values are taken into account:
=# create function int42(cstring) returns int42 AS 'int4in'
language internal strict volatile with (isstrict, iscachable);
CREATE FUNCTION
=# select provolatile from pg_proc where proname = 'int42';
provolatile
-
i
(1 row)
This clause is marked as deprecated since 7.3, so perhaps it would be
time to remove completely its support? It seems to me that this leads to
more confusion than being helpful. And I have not found a trace of code
using those flags on github or such.
Thanks,
--
Michael
From 99a1881bfe605dcb57daab2cb38ef1c7d2805f01 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Mon, 15 Jan 2018 11:20:03 +0900
Subject: [PATCH] Remove support for WITH clause in CREATE FUNCTION
The WITH clause in CREATE FUNCTION has been able to use two parameters,
which have been marked as deprecated in 7.3:
- isStrict, which is equivalent to STRICT
- isCachable, which is equivalent to IMMUTABLE
Specifying in parallel multiple values of IMMUTABLE/STABLE/VOLATILE
results in an error, but using a WITH clause with other keywords gives
priority to the values in the WITH clause, which can be confusing for
users.
---
doc/src/sgml/ref/create_function.sgml | 36 ---
src/backend/commands/functioncmds.c | 55 ---
src/backend/nodes/copyfuncs.c | 1 -
src/backend/nodes/equalfuncs.c| 1 -
src/backend/parser/gram.y | 9 ++
src/include/nodes/parsenodes.h| 1 -
6 files changed, 3 insertions(+), 100 deletions(-)
diff --git a/doc/src/sgml/ref/create_function.sgml b/doc/src/sgml/ref/create_function.sgml
index fd229d1193..c0adb8cf1e 100644
--- a/doc/src/sgml/ref/create_function.sgml
+++ b/doc/src/sgml/ref/create_function.sgml
@@ -37,7 +37,6 @@ CREATE [ OR REPLACE ] FUNCTION
| AS 'definition'
| AS 'obj_file', 'link_symbol'
} ...
-[ WITH ( attribute [, ...] ) ]
@@ -560,41 +559,6 @@ CREATE [ OR REPLACE ] FUNCTION
-
- attribute
-
-
-
- The historical way to specify optional pieces of information
- about the function. The following attributes can appear here:
-
-
-
-isStrict
-
-
- Equivalent to STRICT or RETURNS NULL ON NULL INPUT.
-
-
-
-
-
-isCachable
-
- isCachable is an obsolete equivalent of
- IMMUTABLE; it's still accepted for
- backwards-compatibility reasons.
-
-
-
-
-
-
- Attribute names are not case-sensitive.
-
-
-
-
diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index 12ab33f418..347e25db10 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -788,59 +788,6 @@ compute_attributes_sql_style(ParseState *pstate,
}
-/*-
- * Interpret the parameters *parameters and return their contents via
- * *isStrict_p and *volatility_p.
- *
- * These parameters supply optional information about a function.
- * All have defaults if not specified. Parameters:
- *
- * * isStrict means the function should not be called when any NULL
- * inputs are present; instead a NULL result value should be assumed.
- *
- * * volatility tells the optimizer whether the function's result can
- * be assumed to be repeatable over multiple evaluations.
- *
- */
-static void
-compute_attributes_with_style(ParseState *pstate, bool is_procedure, List *parameters, bool *isStrict_p, char *volatility_p)
-{
- ListCell *pl;
-
- foreach(pl, parameters)
- {
- DefElem*param = (DefElem *) lfirst(pl);
-
- if (pg_strcasecmp(param->defname, "isstrict") == 0)
- {
- if (is_procedure)
-ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("invalid attribute in procedure definition"),
- parser_errposition(pstate, param->location)));
- *isStrict_p = defGetBoolean(param);
- }
- else if (pg_strcasecmp(param->defname, "iscachable") == 0)
- {
- /* obsolete spelling of isImmutable */
- if (is_procedure)
-ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("invalid attribute in procedure definition"),
- parser_errposition
Re: [HACKERS] Useless code in ExecInitModifyTable
Fujita-san, Amit,
* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> On 2017/06/21 16:59, Etsuro Fujita wrote:
> > Commit d3cc37f1d801a6b5cad9bf179274a8d767f1ee50 added this to
> > ExecInitModifyTable:
> >
> > + /* The root table RT index is at the head of the partitioned_rels list
> > */
> > + if (node->partitioned_rels)
> > + {
> > + Index root_rti;
> > + Oid root_oid;
> > +
> > + root_rti = linitial_int(node->partitioned_rels);
> > + root_oid = getrelid(root_rti, estate->es_range_table);
> > + rel = heap_open(root_oid, NoLock); /* locked by InitPlan */
> > + }
> >
> > but I noticed that that function doesn't use the relation descriptor at
> > all. Since partitioned_rels is given in case of an UPDATE/DELETE on a
> > partitioned table, the relation is opened in that case, but the relation
> > descriptor isn't referenced at all in initializing WITH CHECK OPTION
> > constraints and/or RETURNING projections. (The mtstate->resultRelinfo
> > array is referenced in those initialization, instead.) So, I'd like to
> > propose to remove this from that function. Attached is a patch for that.
>
> Thanks for cleaning that up. I cannot see any problem in applying the patch.
Seems like this has gotten a review (and quite a bit of down-stream
discussion that seemed pretty positive), and the latest patch still
applies cleanly and passes the regression tests- is there some reason
it's still marked as Needs Review? Seems like it should really be in
Ready for Committer state.
Amit, if you agree, would you mind going ahead and changing it?
Thanks!
Stephen
signature.asc
Description: PGP signature
Re: [HACKERS] GSoC 2017: weekly progress reports (week 4) and patch for hash index
Greeting Shubham, all, * Michael Paquier (michael.paqu...@gmail.com) wrote: > On Mon, Sep 25, 2017 at 10:34 PM, Shubham Barai > wrote: > > I have attached the rebased version of patch here. > > The patch does not apply and there has been no reviews as well. In > consequence, I am moving it to next CF with "waiting on author" as > status. Please provide a rebased patch. Shubham, would you mind providing an updated patch which applies cleanly, so we can change this to Needs Review and hopefully get someone looking at it? Also, it would be good to respond to Alexander's as to if it would work or not (and perhaps updating the patch accordingly). Otherwise, I'm afriad this patch may end up just getting bumped to the next CF or ending up as 'returned with feedback'. Would be great to get this up to a point where it could be committed. Thanks! Stephen signature.asc Description: PGP signature
Re: [HACKERS] Useless code in ExecInitModifyTable
On 2018/01/15 11:28, Stephen Frost wrote:
> Fujita-san, Amit,
>
> * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
>> On 2017/06/21 16:59, Etsuro Fujita wrote:
>>> Commit d3cc37f1d801a6b5cad9bf179274a8d767f1ee50 added this to
>>> ExecInitModifyTable:
>>>
>>> + /* The root table RT index is at the head of the partitioned_rels list
>>> */
>>> + if (node->partitioned_rels)
>>> + {
>>> + Index root_rti;
>>> + Oid root_oid;
>>> +
>>> + root_rti = linitial_int(node->partitioned_rels);
>>> + root_oid = getrelid(root_rti, estate->es_range_table);
>>> + rel = heap_open(root_oid, NoLock); /* locked by InitPlan */
>>> + }
>>>
>>> but I noticed that that function doesn't use the relation descriptor at
>>> all. Since partitioned_rels is given in case of an UPDATE/DELETE on a
>>> partitioned table, the relation is opened in that case, but the relation
>>> descriptor isn't referenced at all in initializing WITH CHECK OPTION
>>> constraints and/or RETURNING projections. (The mtstate->resultRelinfo
>>> array is referenced in those initialization, instead.) So, I'd like to
>>> propose to remove this from that function. Attached is a patch for that.
>>
>> Thanks for cleaning that up. I cannot see any problem in applying the patch.
>
> Seems like this has gotten a review (and quite a bit of down-stream
> discussion that seemed pretty positive), and the latest patch still
> applies cleanly and passes the regression tests- is there some reason
> it's still marked as Needs Review? Seems like it should really be in
> Ready for Committer state.
+1.
> Amit, if you agree, would you mind going ahead and changing it?
Sure, done.
Thanks,
Amit
Re: [PATCH] Atomic pgrename on Windows
Greetings Alexander, all, * Alexander Korotkov (a.korot...@postgrespro.ru) wrote: > Attached patch atomic-pgrename-windows-1.patch fixes this problem. It > appears to be possible to atomically replace file on Windows – > ReplaceFile() does that. ReplaceFiles() requires target file to exist, > this is why we still need to call MoveFileEx() when it doesn't exist. There's been some discussion on this patch and it seems to, at least, improve the situation on Windows even if it might not completely address the issues. Does anyone else want to take a look and comment, particularly those familiar with the Windows side of things? Otherwise, I'm afraid this patch may end up just sitting in Needs review state fo the entire CF and not getting moved forward, even though it seems like a good improvement for our Windows users. Thanks! Stephen signature.asc Description: PGP signature
Re: PATCH: psql tab completion for SELECT
On January 14, 2018 5:44:01 PM PST, Edmund Horner wrote: >On 15 January 2018 at 14:20, Andres Freund wrote: >> On January 14, 2018 5:12:37 PM PST, Edmund Horner >wrote: >>>And here's a patch to add savepoint protection for tab completion. >> >> It'd be good to explain what that means, so people don't have to read >the patch to be able to discuss whether this is a good idea. > > >Good idea. > >In psql if you have readline support and press TAB, psql will often >run a DB query to get a list of possible completions to type on your >current command line. > >It uses the current DB connection for this, which means that if the >tab completion query fails (e.g. because psql is querying catalog >objects that doesn't exist in your server), the current transaction >(if any) fails. An example of this happening is: Ah, that's what I thought. I don't think this is the right fix. > pg_subscription table doesn't >exist on 9.2! User realises their mistake and types a different >command) > >postgres=# select * from foo; >ERROR: current transaction is aborted, commands ignored until end >of transaction block All worries like this are supposed to check the server version. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)
On Sun, Jan 14, 2018 at 1:43 AM, Peter Geoghegan wrote: > On Sat, Jan 13, 2018 at 4:32 AM, Amit Kapila wrote: >> Yeah, but this would mean that now with parallel create index, it is >> possible that some tuples from the transaction would end up in index >> and others won't. > > You mean some tuples from some past transaction that deleted a bunch > of tuples and committed, but not before someone acquired a still-held > snapshot that didn't see the deleter's transaction as committed yet? > I think I am talking about something different. Let me try to explain in some more detail. Consider a transaction T-1 has deleted two tuples from tab-1, first on page-1 and second on page-2 and committed. There is a parallel transaction T-2 which has an open snapshot/query due to which oldestXmin will be smaller than T-1. Now, in another session, we started parallel Create Index on tab-1 which has launched one worker. The worker decided to scan page-1 and will found that the deleted tuple on page-1 is Recently Dead, so will include it in Index. In the meantime transaction, T-2 got committed/aborted which allows oldestXmin to be greater than the value of transaction T-1 and now leader decides to scan the page-2 with freshly computed oldestXmin and found that the tuple on that page is Dead and has decided not to include it in the index. So, this leads to a situation where some tuples deleted by the transaction will end up in index whereas others won't. Note that I am not arguing that there is any fundamental problem with this, but just want to highlight that such a case doesn't seem to exist with Create Index. > >> The patch uses both parallel_leader_participation and >> force_parallel_mode, but it seems the definition is different from >> what we have in Gather. Basically, even with force_parallel_mode, the >> leader is participating in parallel build. I see there is some >> discussion above about both these parameters and still, there is not >> complete agreement on the best way forward. I think we should have >> parallel_leader_participation as that can help in testing if nothing >> else. > > I think that you're quite right that parallel_leader_participation > needs to be supported for testing purposes. I had some sympathy for > the idea that we should remove leader participation as a worker from > the patch entirely, but the testing argument seems to clinch it. I'm > fine with killing force_parallel_mode, though, because it will be > possible to force the use of parallelism by using the existing > parallel_workers table storage param in the next version of the patch, > regardless of how small the table is. > Okay, this makes sense to me. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [COMMITTERS] pgsql: Improve performance of get_actual_variable_range with recently-d
On Fri, Sep 8, 2017 at 8:41 AM, Tom Lane wrote: > Improve performance of get_actual_variable_range with recently-dead tuples. > > In commit fccebe421, we hacked get_actual_variable_range() to scan the > index with SnapshotDirty, so that if there are many uncommitted tuples > at the end of the index range, it wouldn't laboriously scan through all > of them looking for a live value to return. However, that didn't fix it > for the case of many recently-dead tuples at the end of the index; > SnapshotDirty recognizes those as committed dead and so we're back to > the same problem. > > To improve the situation, invent a "SnapshotNonVacuumable" snapshot type > and use that instead. The reason this helps is that, if the snapshot > rejects a given index entry, we know that the indexscan will mark that > index entry as killed. This means the next get_actual_variable_range() > scan will proceed past that entry without visiting the heap, making the > scan a lot faster. We may end up accepting a recently-dead tuple as > being the estimated extremal value, but that doesn't seem much worse than > the compromise we made before to accept not-yet-committed extremal values. > > The cost of the scan is still proportional to the number of dead index > entries at the end of the range, so in the interval after a mass delete > but before VACUUM's cleaned up the mess, it's still possible for > get_actual_variable_range() to take a noticeable amount of time, if you've > got enough such dead entries. But the constant factor is much much better > than before, since all we need to do with each index entry is test its > "killed" bit. > > We chose to back-patch commit fccebe421 at the time, but I'm hesitant to > do so here, because this form of the problem seems to affect many fewer > people. Also, even when it happens, it's less bad than the case fixed > by commit fccebe421 because we don't get the contention effects from > expensive TransactionIdIsInProgress tests. > > Dmitriy Sarafannikov, reviewed by Andrey Borodin > > Discussion: https://postgr.es/m/05c72cf7-b5f6-4db9-8a09-5ac897653...@yandex.ru > > Branch > -- > master > > Details > --- > https://git.postgresql.org/pg/commitdiff/3ca930fc39ccf987c1c22fd04a1e7463b5dd0dfd > > Modified Files > -- > src/backend/access/heap/heapam.c | 3 +++ > src/backend/utils/adt/selfuncs.c | 40 +++- > src/backend/utils/time/tqual.c | 22 ++ > src/include/utils/snapshot.h | 4 +++- > src/include/utils/tqual.h| 10 ++ > 5 files changed, 65 insertions(+), 14 deletions(-) > > While reading README in nbtree I found a sentence about snapshot that is "There is one minor exception, which is that the optimizer sometimes looks at the boundaries of value ranges using SnapshotDirty". As the snapshot being used has been changed to SnapshotNonVacuumable by this commit should we fix this sentence? Attached a patch for fixing this. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center fix_README_in_nbtree.patch Description: Binary data
Re: [HACKERS] Restricting maximum keep segments by repslots
Hello, At Thu, 11 Jan 2018 12:55:27 +0300, Sergei Kornilov wrote in <2798121515664...@web40g.yandex.ru> > Hello > > >> patch -p1 gives some "Stripping trailing CRs from patch" > >> messages for me, but applied to current HEAD and builds. After > > > > Hmm. I wonder why I get that complaint so often. (It's rather > > common? or caused by the MIME format of my mail?) I'd say with > > confidence that it is because you retrieved the patch file on > > Windows mailer. > I use Debian and web based mailer. Hm, i wget patches from links here > https://www.postgresql.org/message-id/flat/20180111.155910.26212237.horiguchi.kyotaro%40lab.ntt.co.jp > - applies clean both last and previous messages. Its strange. Thanks for the information. The cause I suppose is that *I* attached the files in *text* MIME type. I taught my mailer application to use "Application/Octet-stream" instead and that should make most (or all) people here happy. > Updated patches builds ok, but i found one failed test in make check-world: > contrib/test_decoding/sql/ddl.sql at the end makes SELECT * FROM > pg_replication_slots; which result of course was changed Mmm. Good catch. check-world (contribs) was out of my sight. It is fixed locally. > And i still have no better ideas for naming. I think on something like > if (min_keep_lsn <= restart_lsn) > if (active_pid != 0) > status = "streaming"; > else > status = "keeping"; > else > status = "may_lost"; > This duplicates an existing active field, but I think it's useful as slot > status description. > wal_status streaming/keeping/lost/unknown as described in docs patch is also > acceptable for me. Maybe anyone else has better idea? I'll fix this after the discussion. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
Re: [HACKERS] Restricting maximum keep segments by repslots
Hello,
At Thu, 11 Jan 2018 13:56:14 +, Greg Stark wrote in
> On 11 January 2018 at 09:55, Sergei Kornilov wrote:
> > if (active_pid != 0)
> > status = "streaming";
> > else
> > status = "keeping";
>
> Perhaps "idle" by analogy to a pg_stat_activity entry for a backend
> that's connected but not doing anything.
The state "keeping" is "some segments that are needed by a slot
are still existing but to be removed by the next checkpoint". The
three states are alogogous to green/yellow/red in traffic
lights. "idle" doesn't feel right.
> > status = "may_lost";
>
> Perhaps "stale" or "expired"?
Some random thoughts on this topic:
Reading the field as "WAL record at restrat_lsn is/has been
$(status)", "expired" fits there. "safe"/"crtical"/("stale" and
"expired") would fit "restart_lsn is $(status)"?
If we merge the second sate to the red-side, a boolean column
with the names "wal_preserved" or "wal_available" might work. But
I believe the second state is crucial.
> Is this patch in bike-shed territory? Are there any questions about
> whether we want the basic shape to look like this?
FWIW the summary history of this patch follows.
- added monitoring feature,
- GUC in bytes not in segments,
- show the "min_keep_lsn" instead of "spare amount of avalable
WAL(distance)" (*1)
- changed the words to show the status. (still under discussion)
- added documentation.
I didn't adopt "setting per slot" since the keep amount is not
measured from slot's restart_lsn, but from checkpoint LSN.
*1: As I mentioned upthread, I think that at least the
"pg_replication_slots.min_keep_lsn" is arguable since it shows
the same value for all slots and I haven't found no other
appropriate place.
> Fwiw I think there's a real need for this feature so I would like to
> get it in for Postgres 11.
It encourages me a lot. Thanks.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: Enhance pg_stat_wal_receiver view to display connected host
On Mon, Jan 8, 2018 at 3:32 PM, Haribabu Kommi wrote: > > > On Fri, Jan 5, 2018 at 11:15 PM, Alvaro Herrera > wrote: > >> Haribabu Kommi wrote: >> >> > or >> > >> > write two new functions PQconnhost() and PQconnhostaddr() to return the >> > connected host and hostaddr and reuse the PQport() function. >> >> How about using an API similar to PQconninfo, where we return an array >> of connection options used? Say, PQeffectiveConninfo(). This seems to >> me to reduce ugliness in the API, and be more generally useful. >> > > OK. Added the new API PQeffectiveConninfo() that returns all the connection > options that are actively used. Currently the connection options host, > hostaddr > and port may change based on the active connection and rest of the options > may be same. > > walrecvr could display as an array or just flatten to a string -- not >> sure what's the better option there. > > > Currently I went with a string model to display all the effective_conninfo > options. I feel if we go with string approach, adding a new option that > gets > updated in future is simple. > > postgres=# select conninfo, effective_conninfo from pg_stat_wal_receiver; > -[ RECORD 1 ]--+ > > > - > conninfo | user=kommih passfile=/home/kommih/.pgpass > dbname=replication hostaddr=127.0.0.1,127.0.0.1 port=5434,5432 > application_name=s2 fallback_application_name=walreceiver sslmode=disable > sslcompression=1 target_session_attrs=any > effective_conninfo | user=kommih passfile=/home/kommih/.pgpass > dbname=replication hostaddr=127.0.0.1 port=5432 application_name=s2 > fallback_application_name=walreceiver sslmode=disable sslcompression=1 > target_session_attrs=any > > > Majority of the options are same in both conninfo and effective_conninfo > columns. > Instead of "effective_conninfo" column, how about something like > "remote_server" > as string that displays only the host, hostaddr and port options that > differs with > each connection? > Instead of effective_conninfo, I changed the column name as remote_serve_info and display only the host, hostaddr and port details. These are the only values that differs with each remote connection. patches attached. Regards, Hari Babu Fujitsu Australia 0001-Addition-of-new-libpq-API-PQeffectiveconninfo.patch Description: Binary data 0002-remote_server_info-column-addtion-to-pg_stat_wal_rec.patch Description: Binary data
