> But the xref seems present only in the master/v16/v15 patches, but not
> for the earlier patches v14/v13/v12. Why not?
I missed it.
> But the change was only in the patches v14 onwards. Although the new
> error message was only added for HEAD, isn't it still correct to say
> "A valid version
> We don't expect unrecognized option here and for such a thing, we use
> elog in the code. See the similar usage in
> parseCreateReplSlotOptions().
"pgoutput" is useful for a lot of applications other than our logical
replication subscriber. I think we should expect anything and handle
errors
> Fair enough. I think we should push your first patch only in HEAD as
> this is a minor improvement over the current behaviour. What do you
> think?
I agree.
> This change (Required in between two sentences) looks slightly odd to
> me. Can we instead extend the second line to something like: "This
> parameter is required, and the individual publication names are ...".
> Similarly we can adjust the proto_vesion explanation.
I don't think it's an
> I found the existing error code appropriate because for syntax
> specification, either we need to mandate this at the grammar level or
> at the API level. Also, I think we should give a message similar to an
> existing message: "publication_names parameter missing". For example,
> we can say,
> I saw that the original "publication_names" error was using
> errcode(ERRCODE_INVALID_PARAMETER_VALUE), but TBH since there is no
> option given at all I felt ERRCODE_SYNTAX_ERROR might be more
> appropriate errcode for those 2 mandatory option errors.
It makes sense to me. Changed.
> 2.
>
>
> Thoughts?
I think it'd be really nice to do this without btree_gist.
I imagine something like this:
CREATE INDEX ON tbl USING gist (
range_col,
int_col USING btree
)
I think this would make the index access methods interface more
useful. Index access method developers wouldn't need
> I agree that we missed updating the parameters of the Logical
> Streaming Replication Protocol documentation. I haven't reviewed all
> the details yet but one minor thing that caught my attention while
> looking at your patch is that we can update the exact additional
> information we started to
> To reduce translation efforts, perhaps it is better to arrange for
> these to share a common message.
Good idea. I've done so.
> Also, I am unsure whether to call these "parameters" or "options" -- I
> wanted to call them parameters like in the documentation, but every
> other message in this
I noticed that Logical Streaming Replication Protocol documentation
[1] is missing the options added to "pgoutput" since version 14. A
patch is attached to fix it together with another small one to give a
nice error when "proto_version" parameter is not provided.
[1]
> In fact, the WAL sender always starts reading WAL from restart_lsn,
> which in turn is always <= confirmed_flush_lsn. While reading WAL, WAL
> sender may read XLOG_RUNNING_XACTS WAL record with lsn <=
> confirmed_flush_lsn. While processing XLOG_RUNNING_XACTS record it may
> update its
I was reading the logical replication code and found a little
unnecessary work we are doing.
The confirmed_flushed_lsn cannot reasonably be ahead of the
current_lsn, so there is no point of calling
LogicalConfirmReceivedLocation() every time we update the candidate
xmin or restart_lsn.
Patch is
I tried reviewing the remaining patches. It seems to work correctly,
and passes the tests on my laptop.
> In this pattern I flipped PointerGetDatum(a) to PointerGetDatum(ra.lower),
> because it seems to me correct. I've followed rule of thumb: every sort
> function must extract and use "lower"
> Thanks for the explanation. Attached is a demo code for the hash-join
> case, which is only for PoC to show how we can make it work. It's far
> from complete, at least we need to adjust the cost calculation for this
> 'right anti join'.
I applied the patch and executed some queries. Hash Right
> In future we could have, for instance, LSM or in-memory B-tree or
> other index AM, which could use existing B-tree or hash opclasses.
This would be easily possible with my patch:
CREATE ACCESS METHOD inmemorybtree
TYPE INDEX HANDLER imbthandler
IMPLEMENTS (ordering);
> But even now, we could
> I can see some value perhaps in letting other opclasses refer to
> btree and hash opclasses rather than duplicating their entries.
> But that doesn't seem to be what you're proposing here.
That's what I am proposing. My idea is to change the current btree
and hash opclasses to be under the new
I think we can benefit from higher level operator classes which can
support multiple index implementations. This is achievable by
introducing another type of access method. Here is my idea in SQL:
CREATE ACCESS METHOD ordering
TYPE INTERFACE HANDLER ordering_ifam_handler;
CREATE ACCESS METHOD
> But patch that you propose does not support sorting build added in PG14.
It looks like the change to btree_gist is not committed yet. I'll
rebase my patch once it's committed.
It was a long thread. I couldn't read all of it. Though, the last
patches felt to me like a part of what's already
It could be useful to use bool in exclusion constraints, but it's
currently not nicely supported. The attached patch adds support for
bool to the btree_gist extension, so we can do this.
I am adding this to the commitfest 2021-07.
0001-btree_gist-Support-bool.patch
Description: Binary data
> Please add this patch to the commitfest so that it's not forgotten. It
> will be considered as a new feature so will be considered for commit
> after the next commitfest.
I did [1]. You can add yourself as a reviewer.
> I don't understand why we need to complicate the expressions when
>
The comparison predicates IS [NOT] TRUE/FALSE/UNKNOWN were not
recognised by postgres_fdw, so they were not pushed down to the remote
server. The attached patch adds support for them.
I am adding this to the commitfest 2021-07.
0001-postgres_fdw-Handle-boolean-comparison-predicates.patch
I think this is a good feature that would be useful to JDBC and more.
I don't know the surrounding code very well, but the patch looks good to me.
I agree with Tom Lane that the name of the variable is too verbose.
Maybe "auto_binary_types" is enough. Do we gain much by prefixing
> Could you look into the log files in that test directory what is going
> on?
Command 'test-result-format' not found in
/Users/hasegeli/Developer/postgres/tmp_install/Users/hasegeli/.local/pgsql/bin,
/Users/hasegeli/.local/bin, /opt/homebrew/bin, /usr/local/bin,
/usr/bin, /bin, /usr/sbin, /sbin,
I applied the patch, tried running the test and got the following:
rm -rf
'/Users/hasegeli/Developer/postgres/src/test/modules/libpq_extended'/tmp_check
/bin/sh ../../../../config/install-sh -c -d
'/Users/hasegeli/Developer/postgres/src/test/modules/libpq_extended'/tmp_check
cd . &&
> I've rebased and tested your proposed patch. It seems fine and sensible to me.
Thanks
> I have only one thing to note: as this patch doesn't disable <^ and >^
> operator for boxes the existing state of documentation seem consistent to me:
>
> select '((0,0),(1,1))'::box <<|
> Emre, the CF bot complains that this does not apply anymore, so please
> provide a rebase. By the way, I am a bit confused to see a patch
> that adds new operators on a thread whose subject is about
> documentation.
Rebased version is attached.
The subject is about the documentation, but the
> While revising the docs for the geometric operators, I came across
> these entries:
>
> <^ Is below (allows touching)? circle '((0,0),1)' <^ circle
> '((0,5),1)'
> >^ Is above (allows touching)? circle '((0,5),1)' >^ circle
> >'((0,0),1)'
>
> These have got more than a few
> No, no kind of GUC switch is needed. Just drop underflow/overflow checks.
> You'll get 0 or Infinity in expected places, and infinities are okay since
> 2007.
This is out of scope of this thread. I am not sure opening it to
discussion on another thread would yield any result. Experienced
> Does anyone disagree that that's a bug? Should we back-patch
> a fix, or just change it in HEAD? Given the lack of user
> complaints, I lean a bit towards the latter, but am not sure.
The other functions and operators pay attention to not give an error
when the input is Inf or 0. exp() and
> Perhaps it's too late in the v13 cycle to actually do anything
> about this code-wise, but what should I do documentation-wise?
> I'm certainly not eager to document that these operators behave
> inconsistently depending on which type you're talking about.
I don't think we need to worry too
needed added a
> > > > else if (unlikely(result == 0) && arg1 != 0.0)
> > > > float_underflow_error();
> > >
> > > +1
> >
> > Cool. Emre, any chance you could write a patch along those lines?
>
> Yes, I am happy to do. It makes more
> > > For most places it'd probably end up being easier to read and to
> > > optimize if we just wrote them as
> > > if (unlikely(isinf(result)) && !isinf(arg))
> > > float_overflow_error();
> > > and when needed added a
> > > else if (unlikely(result == 0) && arg1 != 0.0)
> > >
> Wait, no. Didn't we get to the point that we figured out that the
> primary issue is the reversal of the order of what is checked is the
> primary problem, rather than the macro/inline piece?
Reversal of the order makes a small or no difference. The
macro/inline change causes the real slowdown
> Should we update the same macro in contrib/btree_gist/btree_utils_num.h too?
I posted another version incorporating this.
those trigonometric functions don't check for
overflow/underflow like all the rest of float.c. I'll submit another
patch to make them error when overflow/underflow.
The new version is attached.
From fb5052b869255ef9465b1de92e84b2fb66dd6eb3 Mon Sep 17 00:00:00 2001
From: Emre Hasegeli
Date: Fri, 7 F
ce issue, removed
the check_float*_val() functions completely, and added unlikely() as
Tom Lane suggested. It is attached. I confirmed with different
compilers that the macro, and unlikely() makes this noticeably faster.
From e869373ad093e668872f08833de2c5c614aab673 Mon Sep 17 00:00:00 2001
From
> Fwiw, also tried the patch that Kuroda-san had posted yesterday.
I run the same test case too:
clang version 7.0.0:
HEAD 2548.119 ms
with patch 2320.974 ms
clang version 8.0.0:
HEAD 2431.766 ms
with patch 2419.439 ms
clang version 9.0.0:
HEAD 2477.493 ms
with patch 2365.509 ms
gcc
> It's only suggestion, i don't now if somebody wants store empty range without
> bounds.
I thought about the same while developing the BRIN inclusion operator
class. I am not sure how useful empty ranges are in practice, but
keeping their bound would only bring more flexibility, and eliminate
Thank you for your fix.
> This assumes that the merge function returns a newly-palloc'd value.
> That's a shaky assumption; if one of the arguments is an empty range,
> range_merge() returns the other argument, rather than a newly
> constructed value. And surely we can't assume assume that for
>
> I poked at this a bit more. I can reproduce the problem by using
> -mfpmath=387 on dromedary's host (fairly old 32-bit macOS); although
> I also get half a dozen *other* failures in the core regression tests,
> mostly around detection of float overflow. So I'm not quite sure that
> this is
> Looking at the discussion where the feature was added, I think changing the
> EXPLAIN just wasn't considered.
I think this is an oversight. It is very useful to have this on
EXPLAIN.
> The attached patch adds "avoided" to "exact" and "lossy" as a category
> under "Heap Blocks".
It took me a
> It is far from a premature optimization IMO, it is super useful and something
> I was hoping would happen ever since I heard about transition tables being
> worked on.
Me too. Never-ending DELETEs are a common pain point especially for
people migrated from MySQL which creates indexes for
> Surely the comment in line 3839 deserves an update :-)
Done.
> This seems good material. I would put the detailed conventions comment
> separately from the head of the file, like this (where I also changed
> "Type1 *type1" into "Type1 *obj1", and a few "has" to "have")
Looks better to me. I
> Good idea. I haven't though about that, but now such names makes more
> sense to me. I will prepare another patch to improve the naming and
> comments to be applied on top of my current patches.
The patch is attached to improve the comments and variable names. I
explained the functions with
> BTW how did we end up with the regression differences? Presumably you've
> tried that on your machine and it passed. So if we adjust the expected
> file, won't it fail on some other machines?
I had another patch to check for -0 inside float{4,8}_{div,mul}(). I
dropped it on the last set of
> the buildfarm seems to be mostly happy so far, so I've taken a quick
> look at the remaining two parts. The patches still apply, but I'm
> getting plenty of failures in regression tests, due to 0.0 being
> replaced by -0.0.
I think we are better off fixing them locally at the moment like your
> So I basically spent most of the time trying to create a reproducible case
> and I can say I failed. I can however reproduce this with specific large
> data set with specific data distribution, but not an artificial one.
The query plans posted that has the statistics prefer Bitmap Index
Scan.
> I think there are three different things that need to be addressed:
>
> * Underspecified comments.
I agree. My patch added comments, the next one with actual fixes adds
more. I just didn't want to invest even more time on them while the
code is its current shape.
> * The function names and
> Hmmm. It'll be difficult to review such patch without access to a platform
> exhibiting such behavior ... IIRC IBM offers free access to open-source
> devs, I wonder if that would be a way.
I don't have access to such platform either, and I don't know too much
about this business. I left this
> There's still a question here, at least from my perspective, as to which
> is actually going to be faster to perform recovery based off of. A good
> restore command, which pre-fetches the WAL in parallel and gets it local
> and on the same filesystem, meaning that the restore_command only has
> Or perhaps I have it backwards and "l1" and "l2" need to be swapped in
> that description. But the mere fact that there is any question about
> that means that the function is poorly documented and perhaps poorly
> named as well. For that matter, is there a good reason why l1/l2
> have those
> 1) common_entry_cmp is still handling 'delta' as double, although the
> CommonEntry was modified to use float8. IMHO it should also simply call
> float8_cmp_internal instead of doing comparisons.
I am changing it to define delta as "float8" and to use float8_cmp_internal().
> 2)
Currently the startup process tries the "restore_command" before
the WAL files locally available under pg_wal/ [1]. I believe we should
change this behavior.
== The Problem ==
This issue came to our attention after we migrated an application from
an object storage backend, and noticed that
> OK, thanks for confirming. I'll get it committed and we'll see what the
> animals think soon.
Thank you for fixing this. I wanted to preserve this code but wasn't
sure about the correct place or whether it is still necessary.
There are more places we produce -0. The regression tests have
> Isn't the 23040 just the totalpages * 10 per `return totalpages * 10;`
> in bringetbitmap()?
Yes, it is just confusing. The correct value is on one level up of
the tree. It is 204 + 4404 rows removed by index recheck = 4608, so
the estimate is not only 150x but 733x off :(.
The sequential
isnan() function is evidently not present on on Windows
before Visual Studio 2013. We define it on win32_port.h using
_isnan(). However _isnan() is also not present. It is on .
The patch is attached to include this from win32_port.h.
Thanks to Thomas Munro for point this out to me [1]. It is
> The version number restriction isn't strictly needed. I only
> suggested it because it'd match the #if that wraps the code that's
> actually using those macros, introduced by commit cec8394b5ccd. That
> was presumably done because versions >= 1800 (= Visual Studio 2013)
> have their own
> FreeBSD's setproctitle() is a bit slow because it contains a syscall
> or two, so people often run PostgreSQL with update_process_title set
> to off on that OS. That makes the user experience not quite as nice
> as Linux. As a weekend learn-me-some-kernel-hacking project I fixed
> that and got
> Those underscore-prefixed names are defined in Microsoft's
> [3][4]. So now I'm wondering if win32_port.h needs to
> #include if (_MSC_VER < 1800).
I don't have the C experience to decide the correct way. There are
currently many .c files that are including float.h conditionally or
> But now I'm wondering what does this mean for existing indexes? Doesn't this
> effectively mean those are unlikely to give meaningful responses (in the old
> or new semantics)?
The patches shouldn't cause any change to the indexable operators.
The fixes are mostly around the lines and the line
> Thank you, pushed with some editorization and renaming text_startswith to
> starts_with
I am sorry for not noticing this before, but what is the point of this
operator? It seems to me we are only making the prefix searching
business, which is already complicated, more complicated.
Also, the
> After further thought, it seems like the place to deal with this is
> really operator_predicate_proof(), as in the attached draft patch
> against HEAD. This passes the smell test for me, in the sense that
> it's an arguably correct and general extension of the proof rules,
> but it could use
>> I wonder if an alternative to making a cast that can't be immutable,
>> because it looks at a GUC, could be to offer a choice of cast
>> functions: if you need the other behavior, create a schema, do a
>> CREATE CAST in it, and put it on your search path ahead of pg_catalog.
>
> Nope, because
> Thank you for the revised version. This seems fine for me so
> marked this as "Ready for Committer".
Thank you for bearing with me for longer than year.
> By the way I think that line_perp is back patched, could you
> propose it for the older versions? (I already marked my entry as
> Rejected)
> Hmm, state->next refers to two different pointer values on line 1 and line
> 2. It may end up being set to NULL on line 1. Am I missing something?
True, lnext(state->next) can set it to NULL. I confused by the below
code on the same function doing the steps in reverse order. With this
> Patch teaches it to ignore nulls when it's known that the operator being
> used is strict. It is harmless and has the benefit that constraint
> exclusion gives an answer that is consistent with what actually running
> such a qual against a table's rows would do.
Yes, I understood that. I just
>> Shouldn't we check if we consumed all elements (state->next_elem >=
>> state->num_elems) inside the while loop?
>
> You're right. Fixed.
I don't think the fix is correct. arrayconst_next_fn() can still
execute state->next_elem++ without checking if we consumed all
elements. I couldn't
> Yeah, the patch in its current form is wrong, because it will give wrong
> answers if the operator being used in a SAOP is non-strict. I modified
> the patch to consider operator strictness before doing anything with nulls.
I tried to review this patch without any familiarity to the code.
> However, for either patch, I'm a bit concerned about using FPzero()
> on the inner product result. To the extent that the EPSILON bound
> has any useful meaning at all, it needs to mean a maximum difference
> between two coordinate values. The inner product has units of coordinates
> squared,
> I'm a bit confused how this patch has gone through several revisions
> since, but has been marked as "ready for committer" since 2017-09-05. Am
> I missing something?
This is floating between commitfests for longer than a year.
Aleksander Alekseev set it to "ready for committer", but Kyotaro
> - line_eq looks too complex in the normal (not containing NANs)
>cases. We should avoid such complexity if possible.
>
>One problem here is that comparison conceals NANness of
>operands. Conversely arithmetics propagate it. We can converge
>NANness into a number. The attached
> ! circle_contain_pt() does the following comparison and it
> seems to be out of our current policy.
>
> point_dt(center, point) <= radius
>
> I suppose this should use FPle.
>
> FPle(point_dt(center, point), radius)
>
> The same is true of circle_contain_pt(),
> 1."COPT=-DGEODEBUG make" complains as follows.
>
> | geo_ops.c:2445:62: error: invalid type argument of unary ‘*’ (have
> ‘float8 {aka double}’)
> | printf("dist_ppoly_internal- segment 0/n distance is %f\n", *result);
Fixing.
> 2. line_construct_pm has been renamed to line_construct. I
> port.h declares inet_net_ntop and we always compile our own from
> port/inet_net_ntop.c .
There is another copy of it under backend/utils/adt/inet_cidr_ntop.c.
The code looks different but does 90% the same thing. Their naming
and usage is confusing.
I recently needed to format IP addresses
> I'm fine with the current shape. Thanks. Maybe the same
> discussion applies to polygons. (cf. poly_overlap)
It indeed does. I am incorporating.
> line_closept_point asserts line_interpt_line returns true but it
> is hazardous. It is safer if line_closept_point returns NaN if
>
> 0001:
>
> - You removed the check of parallelity check of
> line_interpt_line(old line_interpt_internal) but line_parallel
> is not changed so the consistency between the two functions are
> lost. It is better to be another patch file (maybe 0004?).
I am making line_parallel() use
> I'm not sure what you mean by the "basic comparison ops" but I'm
> fine with the policy, handling each component values in the same
> way with float. So point_eq_point() is right and other functions
> should follow the same policy.
I mean <, >, <= and >= by basic comparison operators.
> 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
> Does this patch series fix bug #14971?
No, it doesn't. I am trying to improve things without touching the EPSILON.
[Replying to an old thread...]
> A customer of ours hit some very slow code while running the
> @>(polygon, polygon) operator with some big polygons. I'm not familiar
> with this stuff but I think the problem is that the algorithm converges
> too slowly to a solution and also has some pretty
> dist_pl is changed to take the smaller distance of both ends of
> the segment. It seems absorbing error, so it might be better
> taking the mean of the two distances. If you have a firm reason
> for the change, it is better to be written there, or it might be
> better left alone.
I am sorry for
81 matches
Mail list logo
