On 2021-01-14 09:00, Michael Paquier wrote:
I don't have more comments by reading the code and my tests have
passed after applying the patch on top of df10ac62. I would have also
added some tests that check after blkno < 0 and > MaxBlockNumber in
all the functions where it can be triggered as th
On Wed, Jan 13, 2021 at 09:27:37AM +0100, Peter Eisentraut wrote:
> Interesting idea. Here is a patch that incorporates that.
Thanks for adding some coverage.
This patch needs a small rebase as Heikki has just introduced some
functions for gist, bumping the module to 1.9 (no need to bump to
1.10
On 2021-01-11 09:09, Michael Paquier wrote:
On Sat, Jan 09, 2021 at 01:41:39PM +0100, Peter Eisentraut wrote:
If we had a way to do such testing then we wouldn't need these markers. But
AFAICT, we don't.
Not sure I am following your point here. Taking your patch, it is
possible to trigger the
On Sat, Jan 09, 2021 at 01:41:39PM +0100, Peter Eisentraut wrote:
> If we had a way to do such testing then we wouldn't need these markers. But
> AFAICT, we don't.
Not sure I am following your point here. Taking your patch, it is
possible to trigger the version of get_raw_page() <= 1.8 just with
On 2021-01-09 02:46, Michael Paquier wrote:
+/* LCOV_EXCL_START */
Does it really make sense to add those markers here? It seems to me
that we would ignore any new coverage if regression tests based on
older versions are added (we may really want to have such tests for
more in-core extensions to
On Fri, Jan 08, 2021 at 04:54:47PM +0100, Peter Eisentraut wrote:
> Updated patch that does that.
Thanks. Looks sane seen from here.
+/* LCOV_EXCL_START */
Does it really make sense to add those markers here? It seems to me
that we would ignore any new coverage if regression tests based on
olde
On 2021-01-08 10:21, Peter Eisentraut wrote:
I think on 64-bit systems it's actually safe, but on 32-bit systems
(with USE_FLOAT8_BYVAL), if you use the new binaries with the old
SQL-level definitions, you'd get the int4 that is passed in interpreted
as a pointer, which would lead to very bad thi
On 2020-12-25 08:45, Michael Paquier wrote:
If we really think that we ought to differentiate, then we could do what
pg_stat_statement does, and have a separate C function that's called
with the obsolete signature (pg_stat_statements_1_8 et al).
With the 1.8 flavor, it is possible to pass down a
On Fri, Dec 04, 2020 at 03:58:22PM -0300, Alvaro Herrera wrote:
> I don't know if it's possible to determine (at function execution time)
> that we're running with the old extension version; if so it might
> suffice to throw a warning but still have the SQL function run the same
> C function.
Hmm.
On 2020-Dec-03, Peter Eisentraut wrote:
> On 2020-11-30 16:32, Alvaro Herrera wrote:
> > On 2020-Nov-30, Peter Eisentraut wrote:
> >
> > > Patch updated this way. I agree it's better that way.
> >
> > Thanks, LGTM.
>
> For a change like this, do we need to change the C symbol names, so that
>
On 2020-11-30 16:32, Alvaro Herrera wrote:
On 2020-Nov-30, Peter Eisentraut wrote:
Patch updated this way. I agree it's better that way.
Thanks, LGTM.
For a change like this, do we need to change the C symbol names, so that
there is no misbehavior if the shared library is not updated at t
On 2020-Nov-30, Peter Eisentraut wrote:
> Patch updated this way. I agree it's better that way.
Thanks, LGTM.
On 2020-11-27 13:37, Ashutosh Bapat wrote:
Yeah, I had it like that for a moment, but then you need to duplicate
the check in get_raw_page() and get_raw_page_fork(). I figured since
get_raw_page_internal() does all the other argument checking also, it
seems sensible to put the bl
On Thu, Nov 26, 2020 at 9:57 PM Peter Eisentraut <
peter.eisentr...@enterprisedb.com> wrote:
> On 2020-11-26 14:27, Alvaro Herrera wrote:
> > On 2020-Nov-26, Peter Eisentraut wrote:
> >
> >> The point of the patch is to have the range check somewhere. If you
> just
> >> cast it, then you won't no
On Wed, Nov 25, 2020 at 8:13 PM Anastasia Lubennikova <
a.lubennik...@postgrespro.ru> wrote:
> On 02.11.2020 18:59, Peter Eisentraut wrote:
> > I have committed 0003.
> >
> > For 0001, normal_rand(), I think you should reject negative arguments
> > with an error.
>
> I've updated 0001. The change
On 2020-11-26 14:27, Alvaro Herrera wrote:
On 2020-Nov-26, Peter Eisentraut wrote:
The point of the patch is to have the range check somewhere. If you just
cast it, then you won't notice out of range arguments. Note that other
contrib modules that take block numbers work the same way.
I'm n
On 2020-Nov-26, Peter Eisentraut wrote:
> The point of the patch is to have the range check somewhere. If you just
> cast it, then you won't notice out of range arguments. Note that other
> contrib modules that take block numbers work the same way.
I'm not saying not to do that; just saying we
On 2020-11-25 20:04, Alvaro Herrera wrote:
On 2020-Nov-25, Peter Eisentraut wrote:
bt_page_stats(PG_FUNCTION_ARGS)
{
text *relname = PG_GETARG_TEXT_PP(0);
- uint32 blkno = PG_GETARG_UINT32(1);
+ int64 blkno = PG_GETARG_INT64(1);
As a matter of
On 2020-Nov-25, Peter Eisentraut wrote:
> bt_page_stats(PG_FUNCTION_ARGS)
> {
> text *relname = PG_GETARG_TEXT_PP(0);
> - uint32 blkno = PG_GETARG_UINT32(1);
> + int64 blkno = PG_GETARG_INT64(1);
As a matter of style, I think it'd be better to have an int6
On 2020-11-02 16:59, Peter Eisentraut wrote:
I have committed 0003.
For 0001, normal_rand(), I think you should reject negative arguments
with an error.
I have committed a fix for that.
For 0002, I think you should change the block number arguments to int8,
same as other contrib modules do.
On 02.11.2020 18:59, Peter Eisentraut wrote:
I have committed 0003.
For 0001, normal_rand(), I think you should reject negative arguments
with an error.
I've updated 0001. The change is trivial, see attached.
For 0002, I think you should change the block number arguments to
int8, same as
I have committed 0003.
For 0001, normal_rand(), I think you should reject negative arguments
with an error.
For 0002, I think you should change the block number arguments to int8,
same as other contrib modules do.
On Fri, 16 Oct 2020 at 19:26, Alvaro Herrera
wrote:
> On 2020-Sep-23, Ashutosh Bapat wrote:
>
> > > You're ignoring the xid use-case, for which DatumGetUInt32 actually is
> > > the right thing.
> >
> > There is DatumGetTransactionId() which should be used instead.
> > That made me search if there
On 2020-Sep-23, Ashutosh Bapat wrote:
> > You're ignoring the xid use-case, for which DatumGetUInt32 actually is
> > the right thing.
>
> There is DatumGetTransactionId() which should be used instead.
> That made me search if there's PG_GETARG_TRANSACTIONID() and yes it's
> there but only defined
On Wed, Sep 23, 2020 at 1:41 AM Tom Lane wrote:
>
> Robert Haas writes:
> > On Mon, Sep 21, 2020 at 3:53 PM Andres Freund wrote:
> >> I think we mostly use it for the few places where we currently expose
> >> data as a signed integer on the SQL level, but internally actually treat
> >> it as a u
Robert Haas writes:
> On Mon, Sep 21, 2020 at 3:53 PM Andres Freund wrote:
>> I think we mostly use it for the few places where we currently expose
>> data as a signed integer on the SQL level, but internally actually treat
>> it as a unsigned data.
> So why is the right solution to that not Dat
On Mon, Sep 21, 2020 at 3:53 PM Andres Freund wrote:
> On 2020-09-21 14:08:22 -0400, Robert Haas wrote:
> > There is no SQL type corresponding to the C data type uint32, so I'm
> > not sure why we even have DatumGetUInt32. I'm sort of suspicious that
> > there's some fuzzy thinking going on there
Hi,
On 2020-09-21 14:08:22 -0400, Robert Haas wrote:
> There is no SQL type corresponding to the C data type uint32, so I'm
> not sure why we even have DatumGetUInt32. I'm sort of suspicious that
> there's some fuzzy thinking going on there.
I think we mostly use it for the few places where we c
Robert Haas writes:
> Typically, the DatumGetBlah() function that you pick should match the
> SQL data type that the function is returning. So if the function
> returns pg_catalog.int4, which corresponds to the C data type int32,
> you would use DatumGetInt32. There is no SQL type corresponding to
On Sun, Sep 20, 2020 at 9:17 PM Hou, Zhijie wrote:
> In (/contrib/bloom/blutils.c:277), I found it use DatumGetInt32 to get UInt32
> type.
> Is it more appropriate to use DatumGetUInt32 here?
Typically, the DatumGetBlah() function that you pick should match the
SQL data type that the function is
On Mon, Sep 21, 2020 at 6:47 AM Hou, Zhijie wrote:
>
> In (/contrib/bloom/blutils.c:277), I found it use DatumGetInt32 to get UInt32
> type.
> Is it more appropriate to use DatumGetUInt32 here?
>
Makes sense. +1 for the patch. I think with the existing code also we
don't have any problem. If we
31 matches
Mail list logo
