Re: WIP/PoC for parallel backup

2019-10-07 Thread Ibrar Ahmed
On Mon, Oct 7, 2019 at 6:06 PM Robert Haas  wrote:

> On Mon, Oct 7, 2019 at 8:48 AM Asif Rehman  wrote:
> > Sure. Though the backup manifest patch calculates and includes the
> checksum of backup files and is done
> > while the file is being transferred to the frontend-end. The manifest
> file itself is copied at the
> > very end of the backup. In parallel backup, I need the list of filenames
> before file contents are transferred, in
> > order to divide them into multiple workers. For that, the manifest file
> has to be available when START_BACKUP
> >  is called.
> >
> > That means, backup manifest should support its creation while excluding
> the checksum during START_BACKUP().
> > I also need the directory information as well for two reasons:
> >
> > - In plain format, base path has to exist before we can write the file.
> we can extract the base path from the file
> > but doing that for all files does not seem a good idea.
> > - base backup does not include the content of some directories but those
> directories although empty, are still
> > expected in PGDATA.
> >
> > I can make these changes part of parallel backup (which would be on top
> of backup manifest patch) or
> > these changes can be done as part of manifest patch and then parallel
> can use them.
> >
> > Robert what do you suggest?
>
> I think we should probably not use backup manifests here, actually. I
> initially thought that would be a good idea, but after further thought
> it seems like it just complicates the code to no real benefit.  I
> suggest that the START_BACKUP command just return a result set, like a
> query, with perhaps four columns: file name, file type ('d' for
> directory or 'f' for file), file size, file mtime. pg_basebackup will
> ignore the mtime, but some other tools might find that useful
> information.
>
> I wonder if we should also split START_BACKUP (which should enter
> non-exclusive backup mode) from GET_FILE_LIST, in case some other
> client program wants to use one of those but not the other.  I think
> that's probably a good idea, but not sure.
>
> I still think that the files should be requested one at a time, not a
> huge long list in a single command.
>

What about have an API to get the single file or list of files? We will use
a single file in
our application and other tools can get the benefit of list of files.

>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
>

-- 
Ibrar Ahmed


Re: fix for BUG #3720: wrong results at using ltree

2019-09-04 Thread Ibrar Ahmed
On Tue, Jul 16, 2019 at 8:52 PM Nikita Glukhov 
wrote:

>
> On 09.07.2019 17:57, Oleg Bartunov wrote:
>
> On Mon, Jul 8, 2019 at 7:22 AM Thomas Munro  
>  wrote:
>
> On Sun, Apr 7, 2019 at 3:46 AM Tom Lane  
>  wrote:
>
> =?UTF-8?Q?Filip_Rembia=C5=82kowski?=  
>  writes:
>
> Here is my attempt to fix a 12-years old ltree bug (which is a todo item).
> I see it's not backward-compatible, but in my understanding that's
> what is documented. Previous behavior was inconsistent with
> documentation (where single asterisk should match zero or more
> labels).http://archives.postgresql.org/pgsql-bugs/2007-11/msg00044.php
>
> [...]
>
>
> In short, I'm wondering if we should treat this as a documentation
> bug not a code bug.  But to do that, we'd need a more accurate
> description of what the code is supposed to do, because the statement
> quoted above is certainly not a match to the actual behavior.
>
> This patch doesn't apply.  More importantly, it seems like we don't
> have a consensus on whether we want it.
>
> Teodor, Oleg, would you like to offer an opinion here?  If I
> understand correctly, the choices are doc change, code/comment change
> or WONT_FIX.  This seems to be an entry that we can bring to a
> conclusion in this CF with some input from the ltree experts.
>
> We are currently very busy and will look at the problem (and dig into
> our memory) later.  There is also another ltree patch
> (https://commitfest.postgresql.org/23/1977/), it would be nice if
> Filip try it.
>
> I looked at "ltree syntax improvement" patch and found two more very
> old bugs in ltree/lquery (fixes are attached):
>
> 1.  ltree/lquery level counter overflow is wrongly checked:
>
> SELECT nlevel((repeat('a.', 65534) || 'a')::ltree);
>  nlevel
> 
>   65535
> (1 row)
>
> -- expected 65536 or error
> SELECT nlevel((repeat('a.', 65535) || 'a')::ltree);
>  nlevel
> 
>   0
> (1 row)
>
> -- expected 65537 or error
> SELECT nlevel((repeat('a.', 65536) || 'a')::ltree);
>  nlevel
> 
>   1
> (1 row)
>
> -- expected 'a...' or error
> SELECT (repeat('a.', 65535) || 'a')::ltree;
>  ltree
> ---
>
> (1 row)
>
> -- expected 'a...' or error
> SELECT (repeat('a.', 65536) || 'a')::ltree;
>  ltree
> ---
>  a
> (1 row)
>
>
> 2.  '*{a}.*{b}.*{c}' is not equivalent to '*{a+b+c}' (as I expect):
>
> SELECT ltree '1.2' ~ '*{2}';
>  ?column?
> --
>  t
> (1 row)
>
> -- expected true
> SELECT ltree '1.2' ~ '*{1}.*{1}';
>      ?column?
> --
>  f
> (1 row)
>
>
> Maybe these two bugs need a separate thread?
>
>
> Please create separate commitfest entry.



> --
> Nikita Glukhov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>
>

-- 
Ibrar Ahmed


Re: fix for BUG #3720: wrong results at using ltree

2019-09-04 Thread Ibrar Ahmed
Please create separate commitfest entry.

Re: Commit fest 2019-09

2019-09-03 Thread Ibrar Ahmed
Hi Alvaro,




On Mon, Sep 2, 2019 at 6:47 PM Alvaro Herrera 
wrote:

> On 2019-Sep-01, Michael Paquier wrote:
>
> > I am not sure if somebody would like to volunteer, but I propose
> > myself as commit fest manager for the next session.  Feel free to
> > reply to this thread if you feel that you could help out as manager
> > for this time.
>
> Hello,
>
> Thanks Michael for handling these tasks over the weekend.
>
> I had offered myself as CFM for this one when the cycle started, so I'm
> happy to do it.
>
> At this time, the commitfest starts with:
>
>  status │ count
> ┼───
>  Needs review   │   165
>  Waiting on Author  │30
>  Ready for Committer│11
>  Returned with Feedback │ 1
>  Moved to next CF   │ 2
>  Committed  │14
>  Rejected   │ 1
>  Withdrawn  │ 4
>
> There's an embarrasingly large number of patches in the "Bug Fixes"
> section.  I encourage reviewers to look at those that "Need Review" so
> that they can set "Ready for Committer" and get a committer to finish
> them up:
>status│ count
> ─┼───
>  Committed   │ 2
>  Needs review│20
>  Ready for Committer │ 2
>  Waiting on Author   │ 4
>  Withdrawn   │ 1
>
> Thanks,
>
> I think it is a lot of work if you want I can help with that. (Just start
getting your messages on threads)

> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>
>

-- 
Ibrar Ahmed


Re: WIP: Data at rest encryption

2019-09-03 Thread Ibrar Ahmed
On Tue, Sep 3, 2019 at 10:21 PM Alvaro Herrera 
wrote:

> On 2019-Aug-02, Shawn Wang wrote:
>
> > Hi Antonin,
> > It is very glad to see the new patch. I used the public patches a long
> time ago.
> > I did some tests like the stream replication, much data running,
> temporary files encryption.
> > I found that there is an issue in the
> src/backend/storage/file/encryption.c. You should put block_size =
> EVP_CIPHER_CTX_block_size(ctx); under the #ifdef USE_ASSERT_CHECKING.
> > There is some problem to merge your patches to the latest kernel in the
> pg_ctl.c.
>
> Is a new, fixed version going to be posted soon?  It's been a while.
>
> Also, apologies if this has been asked before, but: how does this patch
> relate to the stuff being discussed in
> https://postgr.es/m/031401d3f41d$5c70ed90$1552c8b0$@lab.ntt.co.jp ?
>
> Yes, it is related to that, but we don't have that on our agenda in a
weekly meeting.  It has
many conflicting points about what we are doing. Swada / Bruce can comment
on that.


> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>
>

-- 
Ibrar Ahmed


Re: Proposal: roll pg_stat_statements into core

2019-09-03 Thread Ibrar Ahmed
On Tue, Sep 3, 2019 at 11:37 AM Michael Paquier  wrote:

> grOn Mon, Sep 02, 2019 at 12:07:17PM -0400, Tom Lane wrote:
> > Euler Taveira  writes:
> >> At least if pg_stat_statements
> >> was in core you could load it by default and have a GUC to turn it
> >> on/off without restarting the server (that was Magnus proposal and
> >> Andres agreed).
> >
> > That assertion is 100% bogus.  To turn it on or off on-the-fly,
> > you'd need some way to acquire or release its shared memory
> > on-the-fly.
> >
> > It's probably now possible to do something like that, using the
> > DSM mechanisms, but the code for it hasn't been written (AFAIK).
> > And it wouldn't have much to do with whether the module was
> > in core or stayed where it is.
>
> If we were to actually merge the module into core and switch to DSM
> instead of the current fixed amout of shared memory defined at start
> time, then that would be a two-step process: first push the functions
> into code with a GUC_POSTMASTER as currently done, and secondly
> attempt to switch the GUC to be reloadable.
>
> FWIW, I am not sure that we should have the module into core.
> --
> Michael
>

- It's broadly useful.
No doubt it is very useful and most of the customer is using that.

- Right now, the barrier for turning it on is quite high. In addition
  to being non-core, which is already a pretty high barrier at a lot
of organizations,it requires a shared_preload_libraries setting,
  which is pretty close to untenable in a lot of use cases.

We are thinking to move a module in core just because of
"barrier for turning it on is quite high" which is not a very compelling
reason. I am just thinking
why not have a system which makes that easy instead of adding to core.



-- 
Ibrar Ahmed


Re: block-level incremental backup

2019-09-03 Thread Ibrar Ahmed
On Tue, Sep 3, 2019 at 7:39 PM Robert Haas  wrote:

> On Tue, Sep 3, 2019 at 10:05 AM Ibrar Ahmed  wrote:
> > +1 using the library to tar. But I think reason not using tar library is
> TAR is
> > one of the most simple file format. What is the best/newest format of
> TAR?
>
> So, I don't really want to go down this path at all, as I already
> said.  You can certainly do your own research on this topic if you
> wish.
>
> I did that and have experience working on the TAR format.  I was curious
about what
"best/newest" you are talking.



> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>


-- 
Ibrar Ahmed


Re: block-level incremental backup

2019-09-03 Thread Ibrar Ahmed
On Tue, Sep 3, 2019 at 8:00 PM Tom Lane  wrote:

> Ibrar Ahmed  writes:
> > +1 using the library to tar.
>
> Uh, *what* library?
>

I was just replying the Robert that he said

"But I think there must be a reason why tar libraries exist,
and I don't want to write a new one."

I said I am ok to use a library "what he is proposing/thinking",
but explained to him that TAR is the most simpler format that
why PG has its own code.


> pg_dump's pg_backup_tar.c is about 1300 lines, a very large fraction
> of which is boilerplate for interfacing to pg_backup_archiver's APIs.
> The stuff that actually knows specifically about tar looks to be maybe
> a couple hundred lines, plus there's another couple hundred lines of
> (rather duplicative?) code in src/port/tar.c.  None of it is rocket
> science.
>
> I can't believe that it'd be a good tradeoff to create a new external
> dependency to replace that amount of code.  In case you haven't noticed,
> our luck with depending on external libraries has been abysmal.
>
> Possibly there's an argument for refactoring things so that there's
> more stuff in tar.c and less elsewhere, but let's not go looking
> for external code to depend on.
>
> regards, tom lane
>


-- 
Ibrar Ahmed


Re: block-level incremental backup

2019-09-03 Thread Ibrar Ahmed
On Tue, Sep 3, 2019 at 6:00 PM Robert Haas  wrote:

> On Sat, Aug 31, 2019 at 3:41 PM Ibrar Ahmed  wrote:
> > Are we using any tar library in pg_basebackup.c? We already have the
> capability
> > in pg_basebackup to do that.
>
> I think pg_basebackup is using homebrew code to generate tar files,
> but I'm reluctant to do that for reading tar files.  For generating a
> file, you can always emit the newest and "best" tar format, but for
> reading a file, you probably want to be prepared for older or cruftier
> variants.  Maybe not -- I'm not super-familiar with the tar on-disk
> format.  But I think there must be a reason why tar libraries exist,
> and I don't want to write a new one.
>
+1 using the library to tar. But I think reason not using tar library is
TAR is
one of the most simple file format. What is the best/newest format of TAR?

>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>


-- 
Ibrar Ahmed


Re: block-level incremental backup

2019-08-31 Thread Ibrar Ahmed
On Sat, Aug 31, 2019 at 7:59 AM Robert Haas  wrote:

> On Thu, Aug 29, 2019 at 10:41 AM Jeevan Ladhe
>  wrote:
> > Due to the inherent nature of pg_basebackup, the incremental backup also
> > allows taking backup in tar and compressed format. But, pg_combinebackup
> > does not understand how to restore this. I think we should either make
> > pg_combinebackup support restoration of tar incremental backup or
> restrict
> > taking the incremental backup in tar format until pg_combinebackup
> > supports the restoration by making option '--lsn' and '-Ft' exclusive.
> >
> > It is arguable that one can take the incremental backup in tar format,
> extract
> > that manually and then give the resultant directory as input to the
> > pg_combinebackup, but I think that kills the purpose of having
> > pg_combinebackup utility.
>
> I don't agree. You're right that you would have to untar (and
> uncompress) the backup to run pg_combinebackup, but you would also
> have to do that to restore a non-incremental backup, so it doesn't
> seem much different.  It's true that for an incremental backup you
> might need to untar and uncompress multiple prior backups rather than
> just one, but that's just the nature of an incremental backup.  And,
> on a practical level, if you want compression, which is pretty likely
> if you're thinking about incremental backups, the way to get that is
> to use tar format with -z or -Z.
>
> It might be interesting to teach pg_combinebackup to be able to read
> tar-format backups, but I think that there are several variants of the
> tar format, and I suspect it would need to read them all.  If someone
> un-tars and re-tars a backup with a different tar tool, we don't want
> it to become unreadable.  So we'd either have to write our own
> de-tarring library or add an external dependency on one.


Are we using any tar library in pg_basebackup.c? We already have the
capability
in pg_basebackup to do that.



> I don't
> think it's worth doing that at this point; I definitely don't think it
> needs to be part of the first patch.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
>

-- 
Ibrar Ahmed


Re: pg_get_databasebyid(oid)

2019-08-29 Thread Ibrar Ahmed
On Thu, Aug 29, 2019 at 3:16 PM Sergei Kornilov  wrote:

> Hello
>
> > Is there a need for this function for the user?
>
> This was feature request from user. I got such comment:
>
> This function is useful when working with pg_stat_statements. For
> obtaining a databаse name for particular query you need to join pg_database
> relation, but for obtaining an username you just need pg_get_userbyid(). So
> it will be useful not to join extra relation and get a database name using
> the similar function - pg_get_databasebyid().
>
> regards, Sergei
>

Hi,
I think its a user request and don't require to be in the core of
PostgreSQL.
A simple SQL function can fulfill the requirement of the user.

CREATE OR REPLACE FUNCTION pg_get_databasebyid(dboid integer) RETURNS name
AS $$

SELECT datname from pg_database WHERE oid = dboid;
$$ LANGUAGE SQL;

-- 
Ibrar Ahmed


Re: pg_get_databasebyid(oid)

2019-08-29 Thread Ibrar Ahmed
On Wed, Aug 28, 2019 at 6:05 PM Sergei Kornilov  wrote:

> > Please add that to commitfest.
>
> Done: https://commitfest.postgresql.org/24/2261/
>
> regards, Sergei
>
Hi,

I have checked the code, the function "pg_get_userbyid" is used in many
places in code. I am just curious why we need that  "pg_get_databasebyid"
function. Is there a need for this function for the user?


-- 
Ibrar Ahmed


Re: pg_get_databasebyid(oid)

2019-08-28 Thread Ibrar Ahmed
On Wed, Aug 28, 2019 at 5:38 PM Sergei Kornilov  wrote:

> Hello
> We already have function pg_get_userbyid(oid) with lookup in pg_authid
> catalog. My collegue ask me can we add similar function
> pg_get_databasebyid(oid) with lookup in pg_databases.
> It is simple function to get a database name by oid and fallback to
> 'unknown (OID=n)' if missing.
>
> The proposed patch is attached. Currently I missed the tests - I doubt
> which file in src/test/regress/sql/ is the most suitable. pg_get_userbyid
> is called from privileges.sql only.
>
> regards, Sergei


Please add that to commitfest.


-- 
Ibrar Ahmed


Re: pg_upgrade: Error out on too many command-line arguments

2019-08-26 Thread Ibrar Ahmed
On Mon, Aug 26, 2019 at 9:46 AM Michael Paquier  wrote:

> On Sun, Aug 25, 2019 at 05:10:47PM +0200, Julien Rouhaud wrote:
> > I did some searching, and oid2name.c is also missing this.
>
> And pgbench, no?
>

Yes, the patch is slightly different.


> --
> Michael
>


-- 
Ibrar Ahmed


0001-pgbench-Error-out-on-too-many-command-line-argume.patch
Description: Binary data


Re: pg_upgrade: Error out on too many command-line arguments

2019-08-25 Thread Ibrar Ahmed
On Sun, Aug 25, 2019 at 8:39 PM Julien Rouhaud  wrote:

> On Sun, Aug 25, 2019 at 4:30 PM Tom Lane  wrote:
> >
> > Peter Eisentraut  writes:
> > > I propose the attached patch to make pg_upgrade error out on too many
> > > command-line arguments.  This makes it match the behavior of other
> > > PostgreSQL programs.
> >
> > +1 ... are we missing this anywhere else?
>
> I did some searching, and oid2name.c is also missing this.
>
> Yes, "oid2name" missing that check too.



-- 
Ibrar Ahmed


0001-oid2name-Error-out-on-too-many-command-line-argume.patch
Description: Binary data


Re: Email to hackers for test coverage

2019-08-23 Thread Ibrar Ahmed
On Thu, Aug 22, 2019 at 2:46 PM movead...@highgo.ca 
wrote:

> Hello hackers,
>
> One of the area that didn't get much attention in the community
> recently is analysing and increasing the code coverage of
> PostgreSQL regession test suite. I have started working on the
> code coverage by running the GCOV code coverage analysis tool in
> order to analyse the current code coverage and come up with test
> cases to increase the code coverage. This is going to be a long
> exercise so my plan is do it incrementaly. I will be analysing
> some area of untested code and then coming up with test cases to
> test those lines of code in regression and then moving on next
> area of untested code and so on.
>
> So far I have come up with 3 test cases to increase the code
> coverage of PostgreSQL regression test suite.
>
> I have performed the regression run for this exercise on this commit:
> (Commit version 75c1921cd6c868c5995b88113b4463a4830b9a27):
>
> The regression is executed with make check-world command and the
> results are gathered using  'make coverage-html' command.
>
> Below are the lines of untested code that i have analysed and the
> test cases added to regression to test these as part of regression.
>
> *1. src/include/utils/float.h:140*
>
> Analyze:
> This is an error report line when converting a big float8 value
> which a float4 can not storage to float4.
>
> Test case:
> Add a test case as below in file float4.sql:
> select float4(1234567890123456789012345678901234567890::float8);
>
> *2. src/include/utils/float.h:145*
>
> Analyze:
> This is an error report line when converting a small float8 value
> which a float4 can not storage to float4.
>
> Test case:
> Add a test case as below in file float4.sql:
> select float4(0.01::float8);
>
> *3.src/include/utils/sortsupport.h:264*
>
> Analyze:
> It is reverse sorting for the data type that has abbreviated for
> sort, for example macaddr, uuid, numeric, network and I choose
> numeric to do it.
>
> Test cast:
> Add a test case as below in file numeric.sql:
> INSERT INTO num_input_test(n1) values('99.998');
> INSERT INTO num_input_test(n1) values('99.997');
> SELECT * FROM num_input_test ORDER BY n1 DESC;
>
> Result and patch
>
> By adding the test cases, the test coverage of  float.h increased from
> 97.7% to 100% and sortsupport.h increased from 76.7% to 80.0%.
>
> The increase in code coverage can be seen in the before and after
> pictures of GCOV test coverage analysis summary.
>
> The attached patch contain the test cases added in regression for
> increasing the coverage.
>
>
> --
> Movead Li
>

Hi Movead,
Please add that to commitfest.



-- 
Ibrar Ahmed


Re: WIP/PoC for parallel backup

2019-08-23 Thread Ibrar Ahmed
On Fri, Aug 23, 2019 at 10:26 PM Stephen Frost  wrote:

> Greetings,
>
> * Asif Rehman (asifr.reh...@gmail.com) wrote:
> > On Fri, Aug 23, 2019 at 3:18 PM Asim R P  wrote:
> > > Interesting proposal.  Bulk of the work in a backup is transferring
> files
> > > from source data directory to destination.  Your patch is breaking this
> > > task down in multiple sets of files and transferring each set in
> parallel.
> > > This seems correct, however, your patch is also creating a new process
> to
> > > handle each set.  Is that necessary?  I think we should try to achieve
> this
> > > using multiple asynchronous libpq connections from a single basebackup
> > > process.  That is to use PQconnectStartParams() interface instead of
> > > PQconnectdbParams(), wich is currently used by basebackup.  On the
> server
> > > side, it may still result in multiple backend processes per
> connection, and
> > > an attempt should be made to avoid that as well, but it seems
> complicated.
> >
> > Thanks Asim for the feedback. This is a good suggestion. The main idea I
> > wanted to discuss is the design where we can open multiple backend
> > connections to get the data instead of a single connection.
> > On the client side we can have multiple approaches, One is to use
> > asynchronous APIs ( as suggested by you) and other could be to decide
> > between multi-process and multi-thread. The main point was we can extract
> > lot of performance benefit by using the multiple connections and I built
> > this POC to float the idea of how the parallel backup can work, since the
> > core logic of getting the files using multiple connections will remain
> the
> > same, wether we use asynchronous, multi-process or multi-threaded.
> >
> > I am going to address the division of files to be distributed evenly
> among
> > multiple workers based on file sizes, that would allow to get some
> concrete
> > numbers as well as it will also us to gauge some benefits between async
> and
> > multiprocess/thread approach on client side.
>
> I would expect you to quickly want to support compression on the server
> side, before the data is sent across the network, and possibly
> encryption, and so it'd likely make sense to just have independent
> processes and connections through which to do that.
>
> +1 for compression and encryption, but I think parallelism will give us
the benefit with and without the compression.

Thanks,
>
> Stephen
>


-- 
Ibrar Ahmed


Re: WIP/PoC for parallel backup

2019-08-23 Thread Ibrar Ahmed
On Fri, Aug 23, 2019 at 3:18 PM Asim R P  wrote:

> Hi Asif
>
> Interesting proposal.  Bulk of the work in a backup is transferring files
> from source data directory to destination.  Your patch is breaking this
> task down in multiple sets of files and transferring each set in parallel.
> This seems correct, however, your patch is also creating a new process to
> handle each set.  Is that necessary?  I think we should try to achieve this
> using multiple asynchronous libpq connections from a single basebackup
> process.  That is to use PQconnectStartParams() interface instead of
> PQconnectdbParams(), wich is currently used by basebackup.  On the server
> side, it may still result in multiple backend processes per connection, and
> an attempt should be made to avoid that as well, but it seems complicated.
>
> What do you think?
>
> The main question is what we really want to solve here. What is the
bottleneck? and which HW want to saturate?. Why I am saying that because
there are multiple H/W involve while taking the backup (Network/CPU/Disk).
If we
already saturated the disk then there is no need to add parallelism because
we will be blocked on disk I/O anyway.  I implemented the parallel backup
in a sperate
application and has wonderful results. I just skim through the code and have
some reservation that creating a separate process only for copying data is
overkill.
There are two options, one is non-blocking calls or you can have some
worker threads.
But before doing that need to see the pg_basebackup bottleneck, after that,
we
can see what is the best way to solve that. Some numbers may help to
understand the
actual benefit.


-- 
Ibrar Ahmed


Re: max_parallel_workers can't actually be set?

2019-08-17 Thread Ibrar Ahmed
On Sat, Aug 17, 2019 at 10:41 PM Tom Lane  wrote:

> Try this:
> alter system set max_parallel_workers = 20;
> and restart the system.
>
> max_parallel_workers is still 8, according to both SHOW and
> pg_controldata, nor can you launch more than 8 workers.
>
> Even odder, if you just do
>
> regression=# set max_parallel_workers = 200;
> SET
> regression=# show max_parallel_workers;
>  max_parallel_workers
> --
>  200
> (1 row)
>
> which should certainly not happen for a PGC_POSTMASTER parameter.
>
> We seem to have an awful lot of mechanism that's concerned with
> adjustments of max_parallel_workers, for something that apparently
> might as well be a compile-time #define ... so I assume it's supposed
> to be changeable at restart and somebody broke it.  But it's not
> working as I'd expect in any branch from 10 onwards.
>
> regards, tom lane
>
>
>
If I understand that correctly it works for me.

postgres=# alter system set max_parallel_workers = 1;
$ pg_ctl restart -l log
$ psql postgres
psql (13devel)
postgres=# explain analyze select * from test where b > 1;


   QUERY PLAN

---
 Gather  (cost=1000.00..98294.84 rows=1 width=8) (actual
time=1635.959..1636.028 rows=0 loops=1)
   Workers Planned: 2
   Workers Launched: 1
   ->  Parallel Seq Scan on test  (cost=0.00..97294.74 rows=1 width=8)
(actual time=1632.239..1632.239 rows=0 loops=2)
 Filter: (b > 1)
 Rows Removed by Filter: 505

 Planning Time: 0.533 ms
 Execution Time: 1636.080 ms
(8 rows)



postgres=# alter system set max_parallel_workers = 2;
ALTER SYSTEM
postgres=# \q
vagrant@vagrant:~/percona/postgresql$ pg_ctl restart -l log
vagrant@vagrant:~/percona/postgresql$ psql postgres
psql (13devel)
Type "help" for help.
postgres=# explain analyze select * from test where b > 1;
  QUERY PLAN

---
 Gather  (cost=1000.00..98294.84 rows=1 width=8) (actual
time=1622.210..1622.274 rows=0 loops=1)
   Workers Planned: 2
   Workers Launched: 2
   ->  Parallel Seq Scan on test  (cost=0.00..97294.74 rows=1 width=8)
(actual time=1616.000..1616.000 rows=0 loops=3)
 Filter: (b > 1)
     Rows Removed by Filter: 337
 Planning Time: 0.699 ms
 Execution Time: 1622.325 ms
(8 rows)




--
Ibrar Ahmed


Re: Patch: New GUC prepared_statement_limit to limit memory used by prepared statements

2019-08-17 Thread Ibrar Ahmed
On Sat, Aug 17, 2019 at 6:58 PM Daniel Migowski 
wrote:

> Hello,
>
> attached you find a patch that adds a new GUC:
>

Quick questions before looking at the patch.

>
> prepared_statement_limit:
>
>  - Do we have a consensus about the name of GUC? I don't think it is
the right name for that.

 - Is this a WIP patch or the final patch? Because I can see TODO and
non-standard
comments in the patch.



>  Specifies the maximum amount of memory used in each session to
> cache
>  parsed-and-rewritten queries and execution plans. This affects
> the maximum memory
>  a backend threads will reserve when many prepared statements
> are used.
>  The default value of 0 disables this setting, but it is
> recommended to set this
>  value to a bit lower than the maximum memory a backend worker
> thread should reserve
>  permanently.
>
> If the GUC is configured after each save of a CachedPlanSource, or after
> creating a CachedPlan from it, the function
> EnforcePreparedStatementLimit is called now. It checks the mem usage of
> the existing saved CachedPlanSources and invalidates the query_list and
> the gplan if available until the memory limit is met again.
>
> CachedPlanSource are removed-and-tailadded in the saved_plan_list
> everytime GetCachedPlan is called on them so it can be used as a LRU list.
>
> I also reworked ResetPlanCache, PlanCacheRelCallback and
> PlanCacheObjectCallback a bit so when a CachedPlanSource is invalidated
> the query_list is not only marked as invalid but it is also fully
> released to free memory here.
>
> Regards,
> Daniel Migowski
>
> PS@Konstantin: This patch also includes the CachedPlanMemoryUsage
> function you like, maybe you like the review the patch for me?
>
>



-- 
Ibrar Ahmed


Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

2019-08-17 Thread Ibrar Ahmed
On Sat, Aug 17, 2019 at 3:04 AM Bruce Momjian  wrote:

> On Fri, Aug 16, 2019 at 07:47:37PM +0200, Antonin Houska wrote:
> > Bruce Momjian  wrote:
> >
> > > I have seen no one present a clear description of how anything beyond
> > > all-cluster encryption would work or be secure.  Wishing that were not
> > > the case doesn't change things.
> >
> > Since this email thread has grown a lot and is difficult to follow, it
> might
> > help if we summarized various approaches on the wiki, with their pros and
> > cons, and included some links to the corresponding emails in the
> > archive. There might be people who would like think about the problems
> but
> > don't have time to read the whole thread. Overview of the pending
> problems of
> > particular approaches might be useful for newcomers, but also for people
> who
> > followed only part of the discussion. I mean an overview of the storage
> > problems; the key management seems to be less controversial.
> >
> > If you think it makes sense, I can spend some time next week on the
> > research. However I'd need at least an outline of the approaches proposed
> > because I also missed some parts of the thread.
>
> I suggest we schedule a voice call and I will go over all the issues and
> explain why I came to the conclusions listed.  It is hard to know what
> level of detail to explain that in an email, beyond what I have already
> posted on this thread.  The only other options is to read all the emails
> _I_ sent on the thread to get an idea.
>
> +1 for voice call, bruce we usually have a weekly TDE call. I will include
you in
that call.  Currently, in that group are
moon_insung...@lab.ntt.co.jp,
sawada.m...@gmail.com,
shawn.w...@highgo.ca,
ahsan.h...@highgo.ca,
ibrar.ah...@gmail.com

I am able to do that for others as well.
>
> --
>   Bruce Momjian  http://momjian.us
>   EnterpriseDB     http://enterprisedb.com
>
> + As you are, so once was I.  As I am, so you will be. +
> +  Ancient Roman grave inscription +
>


-- 
Ibrar Ahmed


Re: block-level incremental backup

2019-08-16 Thread Ibrar Ahmed
On Fri, Aug 16, 2019 at 4:12 PM Ibrar Ahmed  wrote:

>
>
>
>
> On Fri, Aug 16, 2019 at 3:24 PM Jeevan Chalke <
> jeevan.cha...@enterprisedb.com> wrote:
>
>>
>>
>> On Fri, Aug 2, 2019 at 6:43 PM vignesh C  wrote:
>>
>>> Some comments:
>>> 1) There will be some link files created for tablespace, we might
>>> require some special handling for it
>>>
>>
>> Yep. I have that in my ToDo.
>> Will start working on that soon.
>>
>>
>>> 2)
>>> Retry functionality is hanlded only for copying of full files, should
>>> we handle retry for copying of partial files
>>> 3)
>>> we can have some range for maxretries similar to sleeptime
>>>
>>
>> I took help from pg_standby code related to maxentries and sleeptime.
>>
>> However, as we don't want to use system() call now, I have
>> removed all this kludge and just used fread/fwrite as discussed.
>>
>>
>>> 4)
>>> Should we check for malloc failure
>>>
>>
>> Used pg_malloc() instead. Same is also suggested by Ibrar.
>>
>>
>>>
>>> 5) Should we add display of progress as backup may take some time,
>>> this can be added as enhancement. We can get other's opinion on this.
>>>
>>
>> Can be done afterward once we have the functionality in place.
>>
>>
>>>
>>> 6)
>>> If the backup count increases providing the input may be difficult,
>>> Shall user provide all the incremental backups from a parent folder
>>> and can we handle the ordering of incremental backup internally
>>>
>>
>> I am not sure of this yet. We need to provide the tablespace mapping too.
>> But thanks for putting a point here. Will keep that in mind when I
>> revisit this.
>>
>>
>>>
>>> 7)
>>> Add verbose for copying whole file
>>>
>> Done
>>
>>
>>>
>>> 8) We can also check if approximate space is available in disk before
>>> starting combine backup, this can be added as enhancement. We can get
>>> other's opinion on this.
>>>
>>
>> Hmm... will leave it for now. User will get an error anyway.
>>
>>
>>>
>>> 9)
>>> Combine backup into directory can be combine backup directory
>>>
>> Done
>>
>>
>>>
>>> 10)
>>> MAX_INCR_BK_COUNT can be increased little, some applications use 1
>>> full backup at the beginning of the month and use 30 incremental
>>> backups rest of the days in the month
>>>
>>
>> Yeah, agree. But using any number here is debatable.
>> Let's see others opinion too.
>>
> Why not use a list?
>
>
>>
>>
>>> Regards,
>>> Vignesh
>>> EnterpriseDB: http://www.enterprisedb.com
>>>
>>
>>
>> Attached new sets of patches with refactoring done separately.
>> Incremental backup patch became small now and hopefully more
>> readable than the first version.
>>
>> --
>> Jeevan Chalke
>> Technical Architect, Product Development
>> EnterpriseDB Corporation
>> The Enterprise PostgreSQL Company
>>
>>
>
> +   buf = (char *) malloc(statbuf->st_size);
>
> +   if (buf == NULL)
>
> +   ereport(ERROR,
>
> +   (errcode(ERRCODE_OUT_OF_MEMORY),
>
> +errmsg("out of memory")));
>
> Why are you using malloc, you can use palloc here.
>
>
>
> Hi, I gave another look at the patch and have some quick comments.


-
> char   *extptr = strstr(fn, ".partial");

I think there should be a better and strict way to check the file
extension.

-
> +   extptr = strstr(outfn, ".partial");
> +   Assert (extptr != NULL);

Why are you checking that again, you just appended that in the above
statement?

-
> +   if (verbose && statbuf.st_size > (RELSEG_SIZE * BLCKSZ))
> +   pg_log_info("found big file \"%s\" (size: %.2lfGB): %m",
fromfn,
> +   (double) statbuf.st_size /
(RELSEG_SIZE * BLCKSZ));

This is not just a log, you find a file which is bigger which surely has
some problem.

-
> +* We do read entire 1GB file in memory while taking incremental
backup; so
> +* I don't see any reason why can't we do that here.  Also,
copying data in
> +* chunks is expensive.  However, for bigger files, we still
slice at 1GB
> +* border.


What do you mean by bigger file, a file greater than 1GB? In which case you
get file > 1GB?


-- 
Ibrar Ahmed


Re: [PATCH] Implement INSERT SET syntax

2019-08-16 Thread Ibrar Ahmed
On Fri, Aug 16, 2019 at 8:19 AM Amit Kapila  wrote:

> On Wed, Jul 17, 2019 at 10:00 AM Gareth Palmer 
> wrote:
> >
> > Hello,
> >
> > Attached is a patch that adds the option of using SET clause to specify
> > the columns and values in an INSERT statement in the same manner as that
> > of an UPDATE statement.
> >
> > A simple example that uses SET instead of a VALUES() clause:
> >
> > INSERT INTO t SET c1 = 'foo', c2 = 'bar', c3 = 'baz';
> >
> > Values may also be sourced from a CTE using a FROM clause:
> >
> > WITH x AS (
> >   SELECT 'foo' AS c1, 'bar' AS c2, 'baz' AS c3
> > )
> > INSERT INTO t SET c1 = x.c1, c2 = x.c2, c3 = x.c3 FROM x;
> >
> > The advantage of using the SET clause style is that the column and value
> > are kept together, which can make changing or removing a column or value
> from
> > a large list easier.
> >
> > Internally the grammar parser converts INSERT SET without a FROM clause
> into
> > the equivalent INSERT with a VALUES clause. When using a FROM clause it
> becomes
> > the equivalent of INSERT with a SELECT statement.
> >
> > There was a brief discussion regarding INSERT SET on pgsql-hackers in
> late
> > August 2009 [1].
> >
> > INSERT SET is not part of any SQL standard (that I am aware of), however
> this
> > syntax is also implemented by MySQL [2]. Their implementation does not
> support
> > specifying a FROM clause.
> >
>
> I think this can be a handy feature in some cases as pointed by you,
> but do we really want it for PostgreSQL?  In the last round of
> discussions as pointed by you, there doesn't seem to be a consensus
> that we want this feature.  I guess before spending too much time into
> reviewing this feature, we should first build a consensus on whether
> we need this.
>
>
I agree with you Amit, that we need a consensus on that. Do we really need
that
feature or not. In the previous discussion, there was no resistance to have
that
in PostgreSQL, but some problem with the patch. Current patch is very simple
and not invasive, but still, we need a consensus on that.

Along with users, I request some senior hackers/committers to also
> weigh in about the desirability of this feature.
>
> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>
>
>

-- 
Ibrar Ahmed


Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

2019-08-16 Thread Ibrar Ahmed
On Thu, Aug 15, 2019 at 8:21 PM Bruce Momjian  wrote:

> On Thu, Aug 15, 2019 at 11:24:46AM +0200, Antonin Houska wrote:
> > > I think there are several directions we can go after all-cluster
> > > encryption,
> >
> > I think I misunderstood. What you summarize in
> >
> >
> https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#TODO_for_Full-Cluster_Encryption
> >
>
Do we have any status of TODO's, what has been done and what left? It's
much better if we have a link of discussion of each item.



> > does include
> >
> >
> https://www.postgresql.org/message-id/CAD21AoBjrbxvaMpTApX1cEsO=8N=nc2xVZPB0d9e-VjJ=ya...@mail.gmail.com
> >
> > i.e. per-tablespace keys, right? Then the collaboration should be easier
> than
> > I thought.
>
> No, there is a single tables/indexes key and a WAL key, plus keys for
> rotation.  I explained why per-tablespace keys don't add much value.
>
> --
>   Bruce Momjian  http://momjian.us
>   EnterpriseDB http://enterprisedb.com
>
> + As you are, so once was I.  As I am, so you will be. +
> +  Ancient Roman grave inscription +
>


-- 
Ibrar Ahmed


Re: block-level incremental backup

2019-08-16 Thread Ibrar Ahmed
On Fri, Aug 16, 2019 at 3:24 PM Jeevan Chalke <
jeevan.cha...@enterprisedb.com> wrote:

>
>
> On Fri, Aug 2, 2019 at 6:43 PM vignesh C  wrote:
>
>> Some comments:
>> 1) There will be some link files created for tablespace, we might
>> require some special handling for it
>>
>
> Yep. I have that in my ToDo.
> Will start working on that soon.
>
>
>> 2)
>> Retry functionality is hanlded only for copying of full files, should
>> we handle retry for copying of partial files
>> 3)
>> we can have some range for maxretries similar to sleeptime
>>
>
> I took help from pg_standby code related to maxentries and sleeptime.
>
> However, as we don't want to use system() call now, I have
> removed all this kludge and just used fread/fwrite as discussed.
>
>
>> 4)
>> Should we check for malloc failure
>>
>
> Used pg_malloc() instead. Same is also suggested by Ibrar.
>
>
>>
>> 5) Should we add display of progress as backup may take some time,
>> this can be added as enhancement. We can get other's opinion on this.
>>
>
> Can be done afterward once we have the functionality in place.
>
>
>>
>> 6)
>> If the backup count increases providing the input may be difficult,
>> Shall user provide all the incremental backups from a parent folder
>> and can we handle the ordering of incremental backup internally
>>
>
> I am not sure of this yet. We need to provide the tablespace mapping too.
> But thanks for putting a point here. Will keep that in mind when I revisit
> this.
>
>
>>
>> 7)
>> Add verbose for copying whole file
>>
> Done
>
>
>>
>> 8) We can also check if approximate space is available in disk before
>> starting combine backup, this can be added as enhancement. We can get
>> other's opinion on this.
>>
>
> Hmm... will leave it for now. User will get an error anyway.
>
>
>>
>> 9)
>> Combine backup into directory can be combine backup directory
>>
> Done
>
>
>>
>> 10)
>> MAX_INCR_BK_COUNT can be increased little, some applications use 1
>> full backup at the beginning of the month and use 30 incremental
>> backups rest of the days in the month
>>
>
> Yeah, agree. But using any number here is debatable.
> Let's see others opinion too.
>
Why not use a list?


>
>
>> Regards,
>> Vignesh
>> EnterpriseDB: http://www.enterprisedb.com
>>
>
>
> Attached new sets of patches with refactoring done separately.
> Incremental backup patch became small now and hopefully more
> readable than the first version.
>
> --
> Jeevan Chalke
> Technical Architect, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
>

+   buf = (char *) malloc(statbuf->st_size);

+   if (buf == NULL)

+   ereport(ERROR,

+   (errcode(ERRCODE_OUT_OF_MEMORY),

+errmsg("out of memory")));

Why are you using malloc, you can use palloc here.




-- 
Ibrar Ahmed


Re: UNION ALL

2019-08-15 Thread Ibrar Ahmed
On Fri, Aug 16, 2019 at 12:16 AM <066ce...@free.fr> wrote:

> Generally speaking, when executing UNION ; a DISTINCT is run afterward on
> the resultset.
>
> So, if you're sure that each part of UNION cannot return a line returned
> by another one, you may use UNION ALL, you'll cut the cost of the final
> implicit DISTINCT.
>
>
> - Mail original -
> De: "Mark Pasterkamp" 
> À: pgsql-hackers@lists.postgresql.org
> Envoyé: Jeudi 15 Août 2019 20:37:06
> Objet: UNION ALL
>
>
> Dear all,
>
>
> I was wondering if someone could help me understands what a union all
> actually does.
>
>
> For my thesis I am using Apache Calcite to rewrite queries into using
> materialized views which I then give to a Postgres database.
> For some queries, this means that they will be rewritten in a UNION ALL
> style query between an expression and a table scan of a materialized view.
> However, contrary to what I expected, the UNION ALL query is actually a
> lot slower.
>
>
> As an example, say I have 2 tables: actor and movie. Furthermore, there is
> also a foreign key index on movie to actor.
> I also have a materialized view with the join of these 2 tables for all
> movies <= 2015 called A.
> Now, if I want to query all entries in the join between actor and movie, I
> would assume that a UNION ALL between the join of actor and movie for
> movies >2015 and A is faster than executing the original query..
> If I look at the explain analyze part, I can certainly see a reduction in
> cost up until the UNION ALL part, which carries a respective cost more than
> negating the cost reduction up to a point where I might as well not use the
> existing materialized view.
>
>
> I have some trouble understanding this phenomenon.
> One thought which came to my mind was that perhaps UNION ALL might create
> a temporary table containing both result sets, and then do a table scan and
> return that result.
>
> this would greatly increase IO cost which could attribute to the problem.
> However, I am really not sure what UNION ALL actually does to append both
> result sets so I was wondering if someone would be able to help me out with
> this.
>
>
>
>
> Mark
>
>
> 066ce...@free.fr:  Please, avoid top-posting. It makes harder to follow
the
discussion.

-- 
Ibrar Ahmed


Re: [PATCH] Implement INSERT SET syntax

2019-08-15 Thread Ibrar Ahmed
Patch conflict with this assertion 
Assert(pstate->p_expr_kind == EXPR_KIND_UPDATE_SOURCE); 

src/backend/parser/parse_expr.c line 1570

The new status of this patch is: Waiting on Author


Re: pgbench - add \aset to store results of a combined query

2019-08-15 Thread Ibrar Ahmed
The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:not tested

The patch passed my review, I have not reviewed the documentation changes.

The new status of this patch is: Ready for Committer


Re: pgbench - add \aset to store results of a combined query

2019-08-15 Thread Ibrar Ahmed
On Wed, Jul 10, 2019 at 11:33 AM Fabien COELHO  wrote:

>
> Hello Ibrar,
>
> >>  SELECT 1 AS one \;
> >>  SELECT 2 AS two UNION SELECT 2 \;
> >>  SELECT 3 AS three \aset
> >>
> >> will set both "one" and "three", while "two" is not set because there
> were
> >> two rows. It is a kind of more permissive \gset.
> >
> > Are you sure two is not set :)?
> >
> > SELECT 2 AS two UNION SELECT 2;   -- only returns one row.
> > but
> > SELECT 2 AS two UNION SELECT 10;  -- returns the two rows.
>
> Indeed, my intension was to show an example like the second.
>
> > Is this the expected behavior with \aset?
>
> > In my opinion throwing a valid error like "client 0 script 0 command 0
> > query 0: expected one row, got 2" make more sense.
>
> Hmmm. My intention with \aset is really NOT to throw an error. With
> pgbench, the existence of the variable can be tested later to know whether
> it was assigned or not, eg:
>
>SELECT 1 AS x \;
>-- 2 rows, no assignment
>SELECT 'calvin' AS firstname UNION SELECT 'hobbes' \;
>SELECT 2 AS z \aset
>-- next test is false
>\if :{?firstname}
>  ...
>\endif
>
> The rational is that one may want to benefit from combined queries (\;)
> which result in less communication thus has lower latency, but still be
> interested in extracting some results.
>
> The question is what to do if the query returns 0 or >1 rows. If an error
> is raised, the construct cannot be used for testing whether there is one
> result or not, eg for a query returning 0 or 1 row, you could not write:
>
>\set id random(1, :number_of_users)
>SELECT firtname AS fn FROM user WHERE id = :id \aset
>\if :{?fn}
>  -- the user exists, proceed with further queries
>\else
>  -- no user, maybe it was removed, it is not an error
>\endif
>
> Another option would to just assign the value so that
>   - on 0 row no assignment is made, and it can be tested afterwards.
>   - on >1 rows the last (first?) value is kept. I took last so to
> ensure that all results are received.
>
> I think that having some permissive behavior allows to write some more
> interesting test scripts that use combined queries and extract values.
>
> What do you think?
>
> Yes, I think that make more sense.

> > - With \gset
> >
> > SELECT 2 AS two UNION SELECT 10 \gset
> > INSERT INTO test VALUES(:two,0,0);
> >
> > client 0 script 0 command 0 query 0: expected one row, got 2
> > Run was aborted; the above results are incomplete.
>
> Yes, that is the intented behavior.
>
> > - With \aset
> >
> > SELECT 2 AS two UNION SELECT 10 \aset
> > INSERT INTO test VALUES(:two,0,0);
> > [...]
> > client 0 script 0 aborted in command 1 query 0: ERROR:  syntax error at
> or near ":"
>
> Indeed, the user should test whether the variable was assigned before
> using it if the result is not warranted to return one row.
>
> > The new status of this patch is: Waiting on Author
>
> The attached patch implements the altered behavior described above.
>
> --
> Fabien.



-- 
Ibrar Ahmed


Re: block-level incremental backup

2019-08-13 Thread Ibrar Ahmed
On Mon, Aug 12, 2019 at 4:57 PM Jeevan Chalke <
jeevan.cha...@enterprisedb.com> wrote:

>
>
> On Fri, Aug 9, 2019 at 6:36 PM Robert Haas  wrote:
>
>> On Wed, Aug 7, 2019 at 5:46 AM Jeevan Chalke
>>  wrote:
>> > So, do you mean we should just do fread() and fwrite() for the whole
>> file?
>> >
>> > I thought it is better if it was done by the OS itself instead of
>> reading 1GB
>> > into the memory and writing the same to the file.
>>
>> Well, 'cp' is just a C program.  If they can write code to copy a
>> file, so can we, and then we're not dependent on 'cp' being installed,
>> working properly, being in the user's path or at the hard-coded
>> pathname we expect, etc.  There's an existing copy_file() function in
>> src/backed/storage/file/copydir.c which I'd probably look into
>> adapting for frontend use.  I'm not sure whether it would be important
>> to adapt the data-flushing code that's present in that routine or
>> whether we could get by with just the loop to read() and write() data.
>>
>
> Agree that we can certainly use open(), read(), write(), and close() here,
> but
> given that pg_basebackup.c and basbackup.c are using file operations, I
> think
> using fopen(), fread(), fwrite(), and fclose() will be better here,
> at-least
> for consistetncy.
>

+1 for using  fopen(), fread(), fwrite(), and fclose()


> Let me know if we still want to go with native OS calls.
>
>

-1 for OS call


>
>> --
>> Robert Haas
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>
> --
> Jeevan Chalke
> Technical Architect, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
>

-- 
Ibrar Ahmed


Re: Small const correctness patch

2019-08-08 Thread Ibrar Ahmed
On Fri, Aug 9, 2019 at 1:25 AM Mark G  wrote:

>
>
> On Thu, Aug 8, 2019 at 8:25 PM Ibrar Ahmed  wrote:
>
>> +1
>>
>> Patch successfully applied to the master (
>> 43211c2a02f39d6568496168413dc00e0399dc2e)
>>
>
> That change looks like an unrelated patch for initdb. I'm still not seeing
> my patch there.
>

I said I checked and verified patch against that hash. It applied to that
without any failure. Sorry for the confusion.


>
> -Mark
>
>

-- 
Ibrar Ahmed


Re: Small const correctness patch

2019-08-08 Thread Ibrar Ahmed
+1

Patch successfully applied to the master (
43211c2a02f39d6568496168413dc00e0399dc2e)

On Thu, Aug 8, 2019 at 12:30 PM Mark G  wrote:

> Hello all,
>
> Please find attached a trivial patch making a few arrays const (in
> addition to the data they point to).
>
>
>

-- 
Ibrar Ahmed


Re: initdb: Use varargs macro for PG_CMD_PRINTF

2019-08-07 Thread Ibrar Ahmed
Hi,
Patch does not apply, rebased patch on (
68343b4ad75305391b38f4b42734dc07f2fe7ee2) attached

On Wed, Aug 7, 2019 at 7:04 PM Tom Lane  wrote:

> Peter Eisentraut  writes:
> > Small patch to simplify some no longer necessary complicated code, using
> > varargs macros.
>
> +1
>
> regards, tom lane
>
>
>

-- 
Ibrar Ahmed


initdb-print_v2.patch
Description: Binary data


Re: block-level incremental backup

2019-08-07 Thread Ibrar Ahmed
On Wed, Aug 7, 2019 at 2:47 PM Jeevan Chalke 
wrote:

>
>
> On Mon, Aug 5, 2019 at 7:13 PM Robert Haas  wrote:
>
>> On Fri, Aug 2, 2019 at 9:13 AM vignesh C  wrote:
>> > + rc = system(copycmd);
>>
>> I don't think this patch should be calling system() in the first place.
>>
>
> So, do you mean we should just do fread() and fwrite() for the whole file?
>
> I thought it is better if it was done by the OS itself instead of reading
> 1GB
> into the memory and writing the same to the file.
>
> It is not necessary to read the whole 1GB into Ram.


>
>> --
>> Robert Haas
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>
> --
> Jeevan Chalke
> Technical Architect, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
>

-- 
Ibrar Ahmed


Re: block-level incremental backup

2019-08-06 Thread Ibrar Ahmed
On Tue, Aug 6, 2019 at 11:31 PM Ibrar Ahmed  wrote:

>
> I have not looked at the patch in detail, but just some nits from my side.
>
> On Fri, Aug 2, 2019 at 6:13 PM vignesh C  wrote:
>
>> On Thu, Aug 1, 2019 at 5:06 PM Jeevan Chalke
>>  wrote:
>> >
>> > On Tue, Jul 30, 2019 at 9:39 AM Jeevan Chalke <
>> jeevan.cha...@enterprisedb.com> wrote:
>> >>
>> >> I am almost done writing the patch for pg_combinebackup and will post
>> soon.
>> >
>> >
>> > Attached patch which implements the pg_combinebackup utility used to
>> combine
>> > full basebackup with one or more incremental backups.
>> >
>> > I have tested it manually and it works for all best cases.
>> >
>> > Let me know if you have any inputs/suggestions/review comments?
>> >
>> Some comments:
>> 1) There will be some link files created for tablespace, we might
>> require some special handling for it
>>
>> 2)
>> + while (numretries <= maxretries)
>> + {
>> + rc = system(copycmd);
>> + if (rc == 0)
>> + return;
>>
>> Use API to copy the file instead of "system", better to use the secure
> copy.
>
Ah, it is a local copy, simple copy API is enough.

>
>
>> + pg_log_info("could not copy, retrying after %d seconds",
>> + sleeptime);
>> + pg_usleep(numretries++ * sleeptime * 100L);
>> + }
>> Retry functionality is hanlded only for copying of full files, should
>> we handle retry for copying of partial files
>>
>> The log and the sleep time does not match, you are multiplying sleeptime
> with numretries++ and logging only "sleeptime"
>
> Why we are retiring here, capture proper copy error and act accordingly.
> Blindly retiring does not make sense.
>
> 3)
>> + maxretries = atoi(optarg);
>> + if (maxretries < 0)
>> + {
>> + pg_log_error("invalid value for maxretries");
>> + fprintf(stderr, _("%s: -r maxretries must be >= 0\n"), progname);
>> + exit(1);
>> + }
>> + break;
>> + case 's':
>> + sleeptime = atoi(optarg);
>> + if (sleeptime <= 0 || sleeptime > 60)
>> + {
>> + pg_log_error("invalid value for sleeptime");
>> + fprintf(stderr, _("%s: -s sleeptime must be between 1 and 60\n"),
>> progname);
>> + exit(1);
>> + }
>> + break;
>> we can have some range for maxretries similar to sleeptime
>>
>> 4)
>> + fp = fopen(filename, "r");
>> + if (fp == NULL)
>> + {
>> + pg_log_error("could not read file \"%s\": %m", filename);
>> + exit(1);
>> + }
>> +
>> + labelfile = malloc(statbuf.st_size + 1);
>> + if (fread(labelfile, 1, statbuf.st_size, fp) != statbuf.st_size)
>> + {
>> + pg_log_error("corrupted file \"%s\": %m", filename);
>> + free(labelfile);
>> + exit(1);
>> + }
>> Should we check for malloc failure
>>
>> Use pg_malloc instead of malloc
>
>
>> 5) Should we add display of progress as backup may take some time,
>> this can be added as enhancement. We can get other's opinion on this.
>>
>> Yes, we should, but this is not the right time to do that.
>
>
>> 6)
>> + if (nIncrDir == MAX_INCR_BK_COUNT)
>> + {
>> + pg_log_error("too many incremental backups to combine");
>> + fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
>> progname);
>> + exit(1);
>> + }
>> +
>> + IncrDirs[nIncrDir] = optarg;
>> + nIncrDir++;
>> + break;
>>
>> If the backup count increases providing the input may be difficult,
>> Shall user provide all the incremental backups from a parent folder
>> and can we handle the ordering of incremental backup internally
>>
>> Why we have that limit at first place?
>
>
>> 7)
>> + if (isPartialFile)
>> + {
>> + if (verbose)
>> + pg_log_info("combining partial file \"%s.partial\"", fn);
>> +
>> + combine_partial_files(fn, IncrDirs, nIncrDir, subdirpath, outfn);
>> + }
>> + else
>> + copy_whole_file(infn, outfn);
>>
>> Add verbose for copying whole file
>>
>> 8) We can also check if approximate space is available in disk before
>> starting combine backup, this can be added as enhancement. We can get
>> other's opinion on this.
>>
>> 9)
>> + printf(_("  -i, --incr-backup=DIRECTORY incremental backup directory
>> (maximum %d)\n"), MAX_INCR_BK_COUNT);
>> + printf(_("  -o, --output-dir=DIRECTORY  combine backup into
>> directory\n"));
>> + printf(_("\nGeneral options:\n"));
>> + printf(_("  -n, --no-clean  do not clean up after
>> errors\n"));
>>
>> Combine backup into directory can be combine backup directory
>>
>> 10)
>> +/* Max number of incremental backups to be combined. */
>> +#define MAX_INCR_BK_COUNT 10
>> +
>> +/* magic number in incremental backup's .partial file */
>>
>> MAX_INCR_BK_COUNT can be increased little, some applications use 1
>> full backup at the beginning of the month and use 30 incremental
>> backups rest of the days in the month
>>
>> Regards,
>> Vignesh
>> EnterpriseDB: http://www.enterprisedb.com
>>
>>
>>
>
> --
> Ibrar Ahmed
>


-- 
Ibrar Ahmed


Re: block-level incremental backup

2019-08-06 Thread Ibrar Ahmed
I have not looked at the patch in detail, but just some nits from my side.

On Fri, Aug 2, 2019 at 6:13 PM vignesh C  wrote:

> On Thu, Aug 1, 2019 at 5:06 PM Jeevan Chalke
>  wrote:
> >
> > On Tue, Jul 30, 2019 at 9:39 AM Jeevan Chalke <
> jeevan.cha...@enterprisedb.com> wrote:
> >>
> >> I am almost done writing the patch for pg_combinebackup and will post
> soon.
> >
> >
> > Attached patch which implements the pg_combinebackup utility used to
> combine
> > full basebackup with one or more incremental backups.
> >
> > I have tested it manually and it works for all best cases.
> >
> > Let me know if you have any inputs/suggestions/review comments?
> >
> Some comments:
> 1) There will be some link files created for tablespace, we might
> require some special handling for it
>
> 2)
> + while (numretries <= maxretries)
> + {
> + rc = system(copycmd);
> + if (rc == 0)
> + return;
>
> Use API to copy the file instead of "system", better to use the secure
copy.


> + pg_log_info("could not copy, retrying after %d seconds",
> + sleeptime);
> + pg_usleep(numretries++ * sleeptime * 100L);
> + }
> Retry functionality is hanlded only for copying of full files, should
> we handle retry for copying of partial files
>
> The log and the sleep time does not match, you are multiplying sleeptime
with numretries++ and logging only "sleeptime"

Why we are retiring here, capture proper copy error and act accordingly.
Blindly retiring does not make sense.

3)
> + maxretries = atoi(optarg);
> + if (maxretries < 0)
> + {
> + pg_log_error("invalid value for maxretries");
> + fprintf(stderr, _("%s: -r maxretries must be >= 0\n"), progname);
> + exit(1);
> + }
> + break;
> + case 's':
> + sleeptime = atoi(optarg);
> + if (sleeptime <= 0 || sleeptime > 60)
> + {
> + pg_log_error("invalid value for sleeptime");
> + fprintf(stderr, _("%s: -s sleeptime must be between 1 and 60\n"),
> progname);
> + exit(1);
> + }
> + break;
> we can have some range for maxretries similar to sleeptime
>
> 4)
> + fp = fopen(filename, "r");
> + if (fp == NULL)
> + {
> + pg_log_error("could not read file \"%s\": %m", filename);
> + exit(1);
> + }
> +
> + labelfile = malloc(statbuf.st_size + 1);
> + if (fread(labelfile, 1, statbuf.st_size, fp) != statbuf.st_size)
> + {
> + pg_log_error("corrupted file \"%s\": %m", filename);
> + free(labelfile);
> + exit(1);
> + }
> Should we check for malloc failure
>
> Use pg_malloc instead of malloc


> 5) Should we add display of progress as backup may take some time,
> this can be added as enhancement. We can get other's opinion on this.
>
> Yes, we should, but this is not the right time to do that.


> 6)
> + if (nIncrDir == MAX_INCR_BK_COUNT)
> + {
> + pg_log_error("too many incremental backups to combine");
> + fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
> progname);
> + exit(1);
> + }
> +
> + IncrDirs[nIncrDir] = optarg;
> + nIncrDir++;
> + break;
>
> If the backup count increases providing the input may be difficult,
> Shall user provide all the incremental backups from a parent folder
> and can we handle the ordering of incremental backup internally
>
> Why we have that limit at first place?


> 7)
> + if (isPartialFile)
> + {
> + if (verbose)
> + pg_log_info("combining partial file \"%s.partial\"", fn);
> +
> + combine_partial_files(fn, IncrDirs, nIncrDir, subdirpath, outfn);
> + }
> + else
> + copy_whole_file(infn, outfn);
>
> Add verbose for copying whole file
>
> 8) We can also check if approximate space is available in disk before
> starting combine backup, this can be added as enhancement. We can get
> other's opinion on this.
>
> 9)
> + printf(_("  -i, --incr-backup=DIRECTORY incremental backup directory
> (maximum %d)\n"), MAX_INCR_BK_COUNT);
> + printf(_("  -o, --output-dir=DIRECTORY  combine backup into
> directory\n"));
> + printf(_("\nGeneral options:\n"));
> + printf(_("  -n, --no-clean  do not clean up after
> errors\n"));
>
> Combine backup into directory can be combine backup directory
>
> 10)
> +/* Max number of incremental backups to be combined. */
> +#define MAX_INCR_BK_COUNT 10
> +
> +/* magic number in incremental backup's .partial file */
>
> MAX_INCR_BK_COUNT can be increased little, some applications use 1
> full backup at the beginning of the month and use 30 incremental
> backups rest of the days in the month
>
> Regards,
> Vignesh
> EnterpriseDB: http://www.enterprisedb.com
>
>
>

-- 
Ibrar Ahmed


Re: SQL:2011 PERIODS vs Postgres Ranges?

2019-08-06 Thread Ibrar Ahmed
On Tue, Aug 6, 2019 at 8:28 PM Paul Jungwirth 
wrote:

> Hi Ibrar,
>
> On 8/6/19 3:26 AM, Ibrar Ahmed wrote:
> > - Why we are not allowing any other datatype other than ranges in the
> > primary key. Without that there is no purpose of a primary key.
>
> A temporal primary key always has at least one ordinary column (of any
> type), so it is just a traditional primary key *plus* a PERIOD and/or
> range column to indicate when the record was true.
>
> > - Thinking about some special token to differentiate between normal
> > primary key and temporal primary key
>
> There is already some extra syntax. For the time part of a PK, you say
> `WITHOUT OVERLAPS`, like this:
>
>  CONSTRAINT pk_on_t PRIMARY KEY (id, valid_at WITHOUT OVERLAPS)
>
> In this example `id` is an ordinary column, and `valid_at` is either a
> Postgres range or a SQL:2011 PERIOD. (The latter is not yet implemented
> in my patch but there are some placeholder comments.)
>
> Similarly a foreign key has one or more traditional columns *plus* a
> range/PERIOD. It needs to have a range/PERIOD on both sides. It too has
> some special syntax, but instead of `WITHOUT OVERLAPS` it is `PERIOD`.
> (Don't blame me, I didn't write the standard :-) So here is an example:
>
>  CONSTRAINT fk_t2_to_t FOREIGN KEY (id, PERIOD valid_at)
>REFERENCES t (id, PERIOD valid_at)
>
> You should be able to see my changes to gram.y to support this new syntax.
>
> I hope this clears up how it works! I'm happy to answer more questions
> if you have any. Also if you want to read more:
>
> - This paper by Kulkarni & Michels is a 10-page overview of SQL:2011:
>
>
> https://sigmodrecord.org/publications/sigmodRecord/1209/pdfs/07.industry.kulkarni.pdf
>
> - This is a talk I gave at PGCon 2019 going over the concepts, with a
> lot of pictures. You can find text, slides, and a link to the video here:
>
> https://github.com/pjungwir/postgres-temporal-talk
>
> - This link is ostensibly an annotated bibliography but really tells a
> story about how the research has developed:
>
>
> https://illuminatedcomputing.com/posts/2017/12/temporal-databases-bibliography/
>
> - There is also some discussion about PERIODs vs ranges upthread here,
> as well as here:
>
> https://www.postgresql-archive.org/Periods-td6022563.html
>
>
Thanks, Paul for the explanation.  I think its good start, now I am looking
at the
range_agg patch to integrate that with that and test that.


> Yours,
>
> --
> Paul  ~{:-)
> p...@illuminatedcomputing.com
>


-- 
Ibrar Ahmed


Re: SQL:2011 PERIODS vs Postgres Ranges?

2019-08-06 Thread Ibrar Ahmed
Hi Paul,

On Mon, Aug 5, 2019 at 3:11 AM Paul A Jungwirth 
wrote:

> On Fri, Aug 2, 2019 at 1:49 PM Ibrar Ahmed  wrote:
> > I did some clean-up on this patch. I have also refactored a small
> portion of the code
> > to reduce the footprint of the patch. For simplicity, I have divided the
> patch into 6
> > patches, now it is easy to review and debug.
> > Please follow the PostgreSQL coding guidelines. I have found places
> where you missed that, secondly code even in WIP stage must not have
> WARNING because it looks ugly.
>
> Thank you for the cleanup Ibrar! I'll try to stick to the coding
> standards more closely going forward. If you have any review comments
> I would certainly appreciate them, especially about the overall
> approach. I know that the implementation in its current form is not
> very tasteful, but I wanted to get some feedback on the ideas.
>
> I have reviewed the main design, and in my opinion, it is a good start.

- Why we are not allowing any other datatype other than ranges in the
primary key. Without that there is no purpose of a primary key.

- Thinking about some special token to differentiate between normal
primary key and temporal primary key



Also just to reiterate: this patch depends on my other CF entry
> (range_agg), whose scope has expanded considerably. Right now I'm
> focusing on that. And if you're trying to make this code work, it's
> important to apply the range_agg patch first, since the temporal
> foreign key implementation calls that function.
>
> Also: since this patch raises the question of how to conform to
> SQL:2011 while still supporting Postgres range types, I wrote an
> article that surveys SQL:2011 temporal features in MariaDB, DB2,
> Oracle, and MS SQL Server:
>
> https://illuminatedcomputing.com/posts/2019/08/sql2011-survey/
>
> A few highlights are:
>
> - Everyone lets you define PERIODs, but what you can do with them is
> still *very* limited.
> - No one treats PERIODs as first-class types or expressions; they are
> more like table metadata.
>


> - Oracle PERIODs do permit NULL start/end values, and it interprets
> them as "unbounded". That goes against the standard but since it's
> what Postgres does with ranges, it suggests to me that maybe we should
> follow their lead. Anyway I think a NULL is nicer than a sentinel for
> this purpose.
>

That is an open debate, that we want to strictly follow the standard or map
that
to PostgreSQL range type which allows NULL. But how you will define a
primary
key on that?






>
> Regards,
> Paul
>


-- 
Ibrar Ahmed


Re: [PROPOSAL] Temporal query processing with range types

2019-08-02 Thread Ibrar Ahmed
Hi,
I have rebased the patch and currently reviewing the patch
on master (1e2fddfa33d3c7cc93ca3ee0f32852699bd3e012).




On Mon, Jul 1, 2019 at 4:45 PM Thomas Munro  wrote:

> On Wed, Apr 3, 2019 at 2:12 AM Ibrar Ahmed  wrote:
> > I start looking at the patch, there is a couple of problems with the
> patch. The first one is the OID conflict, which I fixed on my machine. The
> second problem is assertion failure. I think you have not compiled the
> PostgreSQL code with the assertion.
>
> Hi Peter,
>
> Looks like there was some good feedback for this WIP project last time
> around.  It's currently in "Needs Review" status in the July
> Commitfest.  To encourage more review and see some automated compile
> and test results, could we please have a fresh rebase?  The earlier
> patches no longer apply.
>
> Thanks,
>
> --
> Thomas Munro
> https://enterprisedb.com
>


-- 
Ibrar Ahmed


001_temporal_query_processing_with_range_types_v4.patch
Description: Binary data


Re: SQL:2011 PERIODS vs Postgres Ranges?

2019-08-02 Thread Ibrar Ahmed
The patch does not work.

postgres=# CREATE TABLE foo (id int,r int4range, valid_at tsrange, CONSTRAINT 
bar_pk PRIMARY KEY (r, valid_at WITHOUT OVERLAPS));
CREATE TABLE
postgres=# CREATE TABLE bar (id int,r int4range, valid_at tsrange, CONSTRAINT 
bar_fk FOREIGN KEY (r, PERIOD valid_at) REFERENCES foo);
ERROR:  cache lookup failed for type 0

The new status of this patch is: Waiting on Author


Re: block-level incremental backup

2019-07-30 Thread Ibrar Ahmed
On Tue, Jul 30, 2019 at 1:28 AM Robert Haas  wrote:

> On Wed, Jul 10, 2019 at 2:17 PM Anastasia Lubennikova
>  wrote:
> > In attachments, you can find a prototype of incremental pg_basebackup,
> > which consists of 2 features:
> >
> > 1) To perform incremental backup one should call pg_basebackup with a
> > new argument:
> >
> > pg_basebackup -D 'basedir' --prev-backup-start-lsn 'lsn'
> >
> > where lsn is a start_lsn of parent backup (can be found in
> > "backup_label" file)
> >
> > It calls BASE_BACKUP replication command with a new argument
> > PREV_BACKUP_START_LSN 'lsn'.
> >
> > For datafiles, only pages with LSN > prev_backup_start_lsn will be
> > included in the backup.
> > They are saved into 'filename.partial' file, 'filename.blockmap' file
> > contains an array of BlockNumbers.
> > For example, if we backuped blocks 1,3,5, filename.partial will contain
> > 3 blocks, and 'filename.blockmap' will contain array {1,3,5}.
>
> I think it's better to keep both the information about changed blocks
> and the contents of the changed blocks in a single file.  The list of
> changed blocks is probably quite short, and I don't really want to
> double the number of files in the backup if there's no real need. I
> suspect it's just overall a bit simpler to keep everything together.
> I don't think this is a make-or-break thing, and welcome contrary
> arguments, but that's my preference.
>

I had experience working on a similar product and I agree with Robert to
keep
the changed block info and the changed block in a single file make more
sense.
+1

>
> > 2) To merge incremental backup into a full backup call
> >
> > pg_basebackup -D 'basedir' --incremental-pgdata 'incremental_basedir'
> > --merge-backups
> >
> > It will move all files from 'incremental_basedir' to 'basedir' handling
> > '.partial' files correctly.
>
> This, to me, looks like it's much worse than the design that I
> proposed originally.  It means that:
>
> 1. You can't take an incremental backup without having the full backup
> available at the time you want to take the incremental backup.
>
> 2. You're always storing a full backup, which means that you need more
> disk space, and potentially much more I/O while taking the backup.
> You save on transfer bandwidth, but you add a lot of disk reads and
> writes, costs which have to be paid even if the backup is never
> restored.
>
> > 1) Whether we collect block maps using simple "read everything page by
> > page" approach
> > or WAL scanning or any other page tracking algorithm, we must choose a
> > map format.
> > I implemented the simplest one, while there are more ideas:
>
> I think we should start simple.
>
> I haven't had a chance to look at Jeevan's patch at all, or yours in
> any detail, as yet, so these are just some very preliminary comments.
> It will be good, however, if we can agree on who is going to do what
> part of this as we try to drive this forward together.  I'm sorry that
> I didn't communicate EDB's plans to work on this more clearly;
> duplicated effort serves nobody well.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
>

-- 
Ibrar Ahmed


Re: SQL:2011 PERIODS vs Postgres Ranges?

2019-07-30 Thread Ibrar Ahmed
Hi Paul,

I have rebased the patch to master (1e2fddfa33d3c7cc93ca3ee0f32852699bd3e012)
and fixed some compilation warning. Now I am reviewing the actual code.


On Fri, Jul 26, 2019 at 6:35 PM Ibrar Ahmed  wrote:

> The patch requires to rebase on the master branch.
>
> The new status of this patch is: Waiting on Author
>


-- 
Ibrar Ahmed


temporal_pks_fks_v005.patch
Description: Binary data


Re: SQL:2011 PERIODS vs Postgres Ranges?

2019-07-26 Thread Ibrar Ahmed
The patch requires to rebase on the master branch.

The new status of this patch is: Waiting on Author


Re: block-level incremental backup

2019-07-17 Thread Ibrar Ahmed
On Wed, Jul 17, 2019 at 6:43 PM Jeevan Chalke <
jeevan.cha...@enterprisedb.com> wrote:

> On Wed, Jul 17, 2019 at 2:15 PM Ibrar Ahmed  wrote:
>
>>
>> At what stage you will apply the WAL generated in between the START/STOP
>> backup.
>>
>
> In this design, we are not touching any WAL related code. The WAL files
> will
> get copied with each backup either full or incremental. And thus, the last
> incremental backup will have the final WAL files which will be copied as-is
> in the combined full-backup and they will get apply automatically if that
> the data directory is used to start the server.
>

Ok, so you keep all the WAL files since the first backup, right?

>
>
>> --
>> Ibrar Ahmed
>>
>
> --
> Jeevan Chalke
> Technical Architect, Product Development
> EnterpriseDB Corporation
>
>

-- 
Ibrar Ahmed


Re: block-level incremental backup

2019-07-17 Thread Ibrar Ahmed
 and should not look at earlier
> backups for that file. During this phase, we should read only the HEADER of
> each .partial file, building a map of which blocks we're ultimately going
> to
> need to read from each backup. We can also compute the offset within each
> file
> where that block is stored at this stage, again using the header
> information.
>
> 3. Now, we can write the output file - reading each block in turn from the
> correct backup and writing it to the write output file, using the map we
> constructed in the previous step. We should probably keep all of the input
> files open over steps 2 and 3 and then close them at the end because
> repeatedly closing and opening them is going to be expensive. When that's
> done,
> go on to the next file and start over at step 1.
>
>
> At what stage you will apply the WAL generated in between the START/STOP
backup.


> We are already started working on this design.
>
> --
> Jeevan Chalke
> Technical Architect, Product Development
> EnterpriseDB Corporation
>
>

-- 
Ibrar Ahmed


Re: pgbench - extend initialization phase control

2019-07-16 Thread Ibrar Ahmed
The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:not tested

Other than that, the patch looks good to me.

The new status of this patch is: Ready for Committer


Re: pgbench - add \aset to store results of a combined query

2019-07-08 Thread Ibrar Ahmed
>  SELECT 1 AS one \;
>  SELECT 2 AS two UNION SELECT 2 \;
>  SELECT 3 AS three \aset
>
> will set both "one" and "three", while "two" is not set because there were 
> two rows. It is a kind of more permissive \gset.

Are you sure two is not set :)? 
SELECT 2 AS two UNION SELECT 2;   -- only returns one row.
but
SELECT 2 AS two UNION SELECT 10;  -- returns the two rows.


Is this the expected behavior with \aset? In my opinion throwing a valid error 
like "client 0 script 0 command 0 query 0: expected one row, got 2" make more 
sense.


 - With \gset 

SELECT 2 AS two UNION SELECT 10 \gset
INSERT INTO test VALUES(:two,0,0);

$ pgbench postgres -f pgbench_aset.sql -T 1 -j 1 -c 1 -s 10
starting vacuum...end.
client 0 script 0 command 0 query 0: expected one row, got 2
transaction type: pgbench_aset.sql
scaling factor: 10
query mode: simple
number of clients: 1
number of threads: 1
duration: 1 s
number of transactions actually processed: 0
Run was aborted; the above results are incomplete.


- With \aset

SELECT 2 AS two UNION SELECT 10 \aset
INSERT INTO test VALUES(:two,0,0);

vagrant@vagrant:~/percona/postgresql$ pgbench postgres -f pgbench_aset.sql -T 1 
-j 1 -c 1 -s 10
starting vacuum...end.
client 0 script 0 aborted in command 1 query 0: ERROR:  syntax error at or near 
":"
LINE 1: INSERT INTO test VALUES(:two,0,0);
^
transaction type: pgbench_aset.sql
scaling factor: 10
query mode: simple
number of clients: 1
number of threads: 1
duration: 1 s
number of transactions actually processed: 0
Run was aborted; the above results are incomplete.

The new status of this patch is: Waiting on Author


Re: pgbench - extend initialization phase control

2019-06-10 Thread Ibrar Ahmed
Does both client/server side data generation in a single command make sense?

Re: pgbench - add option to show actual builtin script code

2019-05-02 Thread Ibrar Ahmed
Now the patch is good now.

The new status of this patch is: Ready for Committer


Re: pgbench - add option to show actual builtin script code

2019-05-02 Thread Ibrar Ahmed
The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:tested, passed

Patch looks good to me, and work fine on my machine. One minor observation is 
option 'list' mostly used to list the elements like "pgbench -b list" shows the 
available builtin scripts. Therefore we should use a word which seems to be 
more relevant like --show-script.

The new status of this patch is: Waiting on Author


Re: How and at what stage to stop FDW to generate plan with JOIN.

2019-04-23 Thread Ibrar Ahmed
On Wed, Apr 24, 2019 at 1:15 AM Tom Lane  wrote:

> Ibrar Ahmed  writes:
> > I am working on an FDW where the database does not support any operator
> > other than "=" in JOIN condition. Some queries are genrating the plan
> with
> > JOIN having "<" operator. How and at what stage I can stop FDW to not
> make
> > such a plan. Here is my sample query.
>
> What exactly do you think should happen instead?  You can't just tell
> users not to ask such a query.  (Well, you can try, but they'll probably
> go looking for a less broken FDW.)
>
> I know that.


> If what you really mean is you don't want to generate pushed-down
> foreign join paths containing non-equality conditions, the answer is
> to just not do that.  That'd be the FDW's own fault, not that of
> the core planner, if it creates a path representing a join it
> can't actually implement.  You'll end up running the join locally,
> which might not be great, but if you have no other alternative
> then that's what you gotta do.
>
> Yes, that's what I am thinking. In case of non-equality condition join
them locally is
the only solution. I was just confirming.


> If what you mean is you don't know how to inspect the join quals
> to see if you can implement them, take a look at postgres_fdw
> to see how it handles the same issue.
>
> I really don't know postgres_fdw have the same issue, but yes postgres_fdw
is always my starting point.


> regards, tom lane
>


-- 
Ibrar Ahmed


How and at what stage to stop FDW to generate plan with JOIN.

2019-04-23 Thread Ibrar Ahmed
Hi,

I am working on an FDW where the database does not support any operator
other than "=" in JOIN condition. Some queries are genrating the plan with
JOIN having "<" operator. How and at what stage I can stop FDW to not make
such a plan. Here is my sample query.



tpch=# select

l_orderkey,

sum(l_extendedprice * (1 - l_discount)) as revenue,

o_orderdate,

o_shippriority

from

customer,

orders,

lineitem

where

c_mktsegment = 'BUILDING'

and c_custkey = o_custkey

and l_orderkey = o_orderkey

and o_orderdate < date '1995-03-22'

and l_shipdate > date '1995-03-22'

group by

l_orderkey,

o_orderdate,

o_shippriority

order by

revenue,

o_orderdate

LIMIT 10;



   QUERY PLAN


...

Merge Cond: (orders.o_orderkey = lineitem.l_orderkey)

->  Foreign Scan  (cost=1.00..-1.00 rows=1000 width=50)

Output: orders.o_orderdate, orders.o_shippriority, orders.o_orderkey

Relations: (customer) INNER JOIN (orders)

Remote SQL: SELECT r2.o_orderdate, r2.o_shippriority, r2.o_orderkey
FROM  db.customer
r1 ALL INNER JOIN db.orders r2 ON (((r1.c_custkey = r2.o_custkey)) AND
((r2.o_orderdate < '1995-03-22')) AND ((r1.c_mktsegment = 'BUILDING')))
ORDER BY r2.o_orderkey, r2.o_orderdate, r2.o_shippriority

...


--

Ibrar Ahmed


Re: pgbench - add minimal stats on initialization

2019-04-11 Thread Ibrar Ahmed
The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:not tested

Patch works fine on my machine.

The new status of this patch is: Ready for Committer


Re: Commit message / hash in commitfest page.

2019-04-11 Thread Ibrar Ahmed
On Thu, Apr 11, 2019 at 2:44 PM Erikjan Rijkers  wrote:
>
> On 2019-04-11 11:36, Ibrar Ahmed wrote:
> > Hi,
> >
> > Is it possible to have commit-message or at least git hash in
> > commitfest. It will be very easy to track commit against commitfest
> > item.
> >
>
> Commitfest items always point to discussion threads. These threads often
> end with a message that says that the patch is pushed.  IMHO, that
> message would be the place to include the commithash.   It would also be
> easily findable via the commitfest application.
>

+1

> Erik Rijkers
>
>
> > --
> > Ibrar Ahmed



-- 
Ibrar Ahmed




Commit message / hash in commitfest page.

2019-04-11 Thread Ibrar Ahmed
Hi,

Is it possible to have commit-message or at least git hash in
commitfest. It will be very easy to track commit against commitfest
item.

-- 
Ibrar Ahmed




Re: pgbench - add minimal stats on initialization

2019-04-10 Thread Ibrar Ahmed
Hi Fabien,

I have one minor observation that in case of initDropTables you log
'drop' and in case of initCreateTables you log 'create table'. We need
to be consistent. The "drop tables" and "create tables" are the best
fit here. Otherwise, the patch is good.

On Wed, Apr 10, 2019 at 2:18 PM Ibrar Ahmed  wrote:
>
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:   tested, passed
> Spec compliant:   tested, passed
> Documentation:not tested
>
> Please ignore the last email.
>
> Patch works perfectly and the code is well-written. I have one minor 
> observation that in case of initDropTables you log "drop" and in case of 
> initCreateTables you log "create table". I think you need to be consistent. 
> And why not "drop tables" and "create tables"
>
> The new status of this patch is: Waiting on Author



-- 
Ibrar Ahmed




Re: pgbench - add minimal stats on initialization

2019-04-10 Thread Ibrar Ahmed
The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:not tested

Please ignore the last email.

Patch works perfectly and the code is well-written. I have one minor 
observation that in case of initDropTables you log "drop" and in case of 
initCreateTables you log "create table". I think you need to be consistent. And 
why not "drop tables" and "create tables"

The new status of this patch is: Waiting on Author

Re: pgbench - add minimal stats on initialization

2019-04-10 Thread Ibrar Ahmed
The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:   tested, failed
Spec compliant:   tested, failed
Documentation:not tested

Patch works perfectly and the code is well-written. I have one minor 
observation that in case of initDropTables you log "drop" and in case of 
initCreateTables you log "create table". I think you need to be consistent. And 
why not "drop tables" and "create tables"

The new status of this patch is: Waiting on Author


Re: dropdb --force

2019-04-09 Thread Ibrar Ahmed
The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:not tested

The feature works fine on my machine. The code is well-written.

The new status of this patch is: Ready for Committer


Re: dropdb --force

2019-04-09 Thread Ibrar Ahmed
Yes, I think it is because of this code Snippet

if (force_terminate)
{
/* try to terminate backend */
#ifdef HAVE_SETSID
kill(-(proc->pid), SIGTERM);
#else
kill(proc->pid, SIGTERM);



On Tue, Apr 9, 2019 at 3:06 PM Ibrar Ahmed  wrote:
>
> Is this the intended behavior? SIGTERM is received.
>
> test=# begin;
> BEGIN
> test=# create table test(a int);
> CREATE TABLE
>
>
> In another terminal drop the database.
>
> test=# begin;
> psql: FATAL:  terminating connection due to administrator command
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> !>



-- 
Ibrar Ahmed




Re: dropdb --force

2019-04-09 Thread Ibrar Ahmed
Is this the intended behavior? SIGTERM is received.

test=# begin;
BEGIN
test=# create table test(a int);
CREATE TABLE


In another terminal drop the database.

test=# begin;
psql: FATAL:  terminating connection due to administrator command
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>

Re: [PROPOSAL] Temporal query processing with range types

2019-04-02 Thread Ibrar Ahmed
I start looking at the patch, there is a couple of problems with the patch. The 
first one is the OID conflict, which I fixed on my machine. The second problem 
is assertion failure. I think you have not compiled the PostgreSQL code with 
the assertion. 

...
postgres=# SELECT *
FROM (projects p1 NORMALIZE projects p2 USING() WITH(t,t)) p_adjusted;
TRAP: FailedAssertion("!(ptr == ((void *)0) || (((const Node*)(ptr))->type) == 
type)", File: "../../../src/include/nodes/nodes.h", Line: 588)
psql: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: 2019-04-02 
12:50:09.654 UTC [27550] LOG:  server process (PID 27559) was terminated by 
signal 6: Aborted
...

Although this patch is WIP, but please avoid mix declaration to avoid the 
compiler warning message.

...
joinpath.c:993:3: warning: ISO C90 forbids mixed declarations and code 
[-Wdeclaration-after-statement]
   PathTarget *target_split = makeNode(PathTarget);
...

I am still looking at the patch.

Re: Problem with default partition pruning

2019-03-04 Thread Ibrar Ahmed
Hi  Yuzuko Hosoya,

Ignore my last message, I think this is also a legitimate scan on default
partition.


On Mon, Mar 4, 2019 at 10:29 PM Ibrar Ahmed  wrote:

> Hi
>
> Patch work fine to me, but I have one test case where default partition
> still scanned.
>
> postgres=# explain select * from test1 where (id < 10) and true;
> QUERY PLAN
> ---
>  Append  (cost=0.00..55.98 rows=846 width=36)
>->  Seq Scan on test1_1  (cost=0.00..25.88 rows=423 width=36)
>  Filter: (id < 10)
>->  Seq Scan on test1_def  (cost=0.00..25.88 rows=423 width=36)
>  Filter: (id < 10)
> (5 rows)



-- 
Ibrar Ahmed


Re: Problem with default partition pruning

2019-03-04 Thread Ibrar Ahmed
Hi 

Patch work fine to me, but I have one test case where default partition still 
scanned. 

postgres=# explain select * from test1 where (id < 10) and true;
QUERY PLAN 
---
 Append  (cost=0.00..55.98 rows=846 width=36)
   ->  Seq Scan on test1_1  (cost=0.00..25.88 rows=423 width=36)
 Filter: (id < 10)
   ->  Seq Scan on test1_def  (cost=0.00..25.88 rows=423 width=36)
 Filter: (id < 10)
(5 rows)

Re: \describe*

2019-03-04 Thread Ibrar Ahmed
Hi Corey,

Here is the modified patch (sample).



On Mon, Mar 4, 2019 at 7:02 PM Ibrar Ahmed  wrote:

> The following review has been posted through the commitfest application:
> make installcheck-world:  not tested
> Implements feature:   not tested
> Spec compliant:   not tested
> Documentation:not tested
>
> Thanks for the patch, I have reviewed the patch and have some comments
> about the patch. The review includes the testing of the patch along with
> some code review.
>
> Here are my testings results,
>
> - Tab completion for \descibe-verbose.
> I know that \d+ tab completion is also not there, but I think we must have
> tab completion for \descibe-verbose.
>
> postgres=# \describe-
> \describe-extension
>  \describe-replication-publication \describe-user-mapping
> \describe-foreign-data-wrapper
> \describe-replication-subscription\describe-view
> \describe-foreign-server  \describe-role
>   \describe-window-function
> \describe-foreign-table   \describe-rule
>  ...
>
>
> - Error message in each command.
> There is an error message after each command, here is the example.
> postgres=# \describe
> List of relations
>  Schema | Name | Type  |  Owner
> +--+---+-
>  public | foo  | table | vagrant
>
> (1 row)
> Invalid command \describe. Try \? for help.
>
>
> I think this status is causing the problem.
>
>
>
> +   /*
> standard listing of interesting things */
> +   success =
> listTables("tvmsE", NULL, show_verbose, show_system);
> +   }
> +   status = PSQL_CMD_UNKNOWN;
>
>
>
>
> - Confusion about \desc and \desC
> There is confusion while running the \desc command. I know the problem,
> but the user may confuse by this.
> postgres=# \desC
>List of foreign servers
>  Name | Owner | Foreign-data wrapper
> --+---+--
> (0 rows)
>
> postgres=# \desc
> Invalid command \desc. Try \? for help.
>
> - Auto-completion of commands.
> There is some more confusion in the completion of commands.
>
> This command shows List of aggregates.
> postgres=# \describe-aggregate-function
>  List of aggregate functions
>  Schema | Name | Result data type | Argument data types | Description
> +--+--+-+-
> (0 rows)
>
>
>
> This command shows a list of relation "\d"
> postgres=# \describe-aggregatE-function
> List of relations
>  Schema | Name | Type  |  Owner
> +--+---+-
>  public | foo  | table | vagrant
> (1 row)
>
> This command also shows a list of relations "\d".
> postgres=# \describe-aggr
> List of relations
>  Schema | Name | Type  |  Owner
> +--+---+-
>  public | foo  | table | vagrant
> (1 row)
>
> This command shows error messages.
> postgres=# \descr
> Invalid command \descr. Try \? for help.
>
> ...
>
>
> Code review.
> -
>
> I have done a brief code review except for the documentation code. I don't
> like this code
>
> if (cmd_match(cmd,"describe-aggregate-function"))
>
>  success = describeAggregates(pattern, show_verbose, show_system);
>  else if (cmd_match(cmd,
> "describe-access-method"))
>  success = describeAccessMethods(pattern,
> show_verbose);
>  else if (cmd_match(cmd,
> "describe-tablespace"))
>  success = describeTablespaces(pattern,
> show_verbose);
>  else if (cmd_match(cmd,
> "describe-conversion"))
>      success = listConversions(pattern,
> show_verbose, show_system);
>  else if (cmd_match(cmd, "describe-cast"))
>  success = listCasts(pattern, show_verbose
>
>
> This can be achieved with the list/array/hash table, so I have changed
> that code in the attached patch just for a sample if you want I can do that
> for whole code.
>
> --
> Ibrar Ahmed
>
> The new status of this patch is: Waiting on Author
>


-- 
Ibrar Ahmed


0001-Add-describe-commands-to-compliment-d-commands-ibrar-v2.patch
Description: Binary data


Re: \describe*

2019-03-04 Thread Ibrar Ahmed
The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

Thanks for the patch, I have reviewed the patch and have some comments about 
the patch. The review includes the testing of the patch along with some code 
review. 

Here are my testings results, 

- Tab completion for \descibe-verbose.
I know that \d+ tab completion is also not there, but I think we must have tab 
completion for \descibe-verbose.

postgres=# \describe-
\describe-extension   \describe-replication-publication 
\describe-user-mapping
\describe-foreign-data-wrapper\describe-replication-subscription
\describe-view
\describe-foreign-server  \describe-role
\describe-window-function
\describe-foreign-table   \describe-rule  
 ...


- Error message in each command.
There is an error message after each command, here is the example. 
postgres=# \describe
List of relations
 Schema | Name | Type  |  Owner  
+--+---+-
 public | foo  | table | vagrant

(1 row)
Invalid command \describe. Try \? for help.


I think this status is causing the problem.



+   /* standard 
listing of interesting things */
+   success = 
listTables("tvmsE", NULL, show_verbose, show_system);
+   }
+   status = PSQL_CMD_UNKNOWN;




- Confusion about \desc and \desC
There is confusion while running the \desc command. I know the problem, but the 
user may confuse by this.
postgres=# \desC
   List of foreign servers
 Name | Owner | Foreign-data wrapper 
--+---+--
(0 rows)

postgres=# \desc
Invalid command \desc. Try \? for help.

- Auto-completion of commands.
There is some more confusion in the completion of commands. 

This command shows List of aggregates.
postgres=# \describe-aggregate-function 
 List of aggregate functions
 Schema | Name | Result data type | Argument data types | Description 
+--+--+-+-
(0 rows)



This command shows a list of relation "\d" 
postgres=# \describe-aggregatE-function 
List of relations
 Schema | Name | Type  |  Owner  
+--+---+-
 public | foo  | table | vagrant
(1 row)

This command also shows a list of relations "\d".
postgres=# \describe-aggr 
List of relations
 Schema | Name | Type  |  Owner  
+--+---+-
 public | foo  | table | vagrant
(1 row)

This command shows error messages.
postgres=# \descr
Invalid command \descr. Try \? for help.

...


Code review. 
-

I have done a brief code review except for the documentation code. I don't like 
this code 

if (cmd_match(cmd,"describe-aggregate-function"))   
 
 success = describeAggregates(pattern, show_verbose, show_system);
 else if (cmd_match(cmd, "describe-access-method"))
 success = describeAccessMethods(pattern, 
show_verbose);
 else if (cmd_match(cmd, "describe-tablespace"))
 success = describeTablespaces(pattern, 
show_verbose);
 else if (cmd_match(cmd, "describe-conversion"))
 success = listConversions(pattern, 
show_verbose, show_system);
 else if (cmd_match(cmd, "describe-cast"))
 success = listCasts(pattern, show_verbose


This can be achieved with the list/array/hash table, so I have changed that 
code in the attached patch just for a sample if you want I can do that for 
whole code.

-- 
Ibrar Ahmed

The new status of this patch is: Waiting on Author


Re: Temporal Table Proposal

2019-02-25 Thread Ibrar Ahmed
Hi Paul,

On Sat, Feb 23, 2019 at 2:16 AM Paul Jungwirth 
wrote:

> On 2/22/19 11:31 AM, Euler Taveira wrote:
> > Em sex, 22 de fev de 2019 às 15:41, Ibrar Ahmed
> >  escreveu:
> >>
> >> While working on another PostgreSQL feature, I was thinking that we
> could use a temporal table in PostgreSQL. Some existing databases offer
> this. I searched for any discussion on the PostgreSQL mailing list, but
> could not find any. Maybe my search wasn’t accurate enough:  if anyone can
> point me to a discussion, that would be useful.
> >>
> >
> https://www.postgresql.org/message-id/CA%2BrenyUb%2BXHzsrPHHR6ELqguxaUPGhOPyVc7NW%2BkRsRpBZuUFQ%40mail.gmail.com
> >
> > This is the last one. I don't know why it wasn't in the January CF.
>
> Oh that's by me! :-)
>
> I didn't put it into the CF because I wanted to get some feedback on
> primary keys before I got too far into foreign keys, but someone
> recently advised me to starting adding to CFs anyway with "WIP" in the
> title, so I'll do that next time.
>
> Btw my own patch is very modest, and I'd love to see this other much
> more extensive patch get some attention:
>
>
> https://www.postgresql.org/message-id/flat/CAHO0eLYyvuqwF%3D2FsgDn1xOs_NOrFBu9Xh-Wq%2BaWfFy0y6%3DjWQ%40mail.gmail.com#4f7fbace3a2f2ce85fcc161cc3fdd273
>
> They were told to adjust where in the query pipeline they do their work,
> and the latest patch does that (as I understand it), but I don't think
> anyone has looked at it yet.
>
> Both of these patches use range types rather than SQL:2011 PERIODs, but
> I'd like to *also* support PERIODs (and accept ranges everywhere we
> accept PERIODs). Vik Fearing already has a patch to let you *declare*
> PERIODs:
>
> https://www.postgresql-archive.org/Periods-td6022563.html
>
> Actually using PERIODs in queries seems like a decent chunk of work
> though: basically it means making our grammar & processing accept
> PERIODs anywhere they currently accept columns. I'd love to hear some
> thoughts/suggestions around that. For example: a PERIOD is *similar* to
> a GENERATED column, so maybe the work being done there can/should
> influence how we implement them.
>
> I'm excited to be getting some momentum around temporal features though!
> I'm supposed to give a talk about them at PGCon in Ottawa this spring,
> so hopefully that will help too.
>
> Yours,
>
> --
> Paul  ~{:-)
> p...@illuminatedcomputing.com
>
> Great, to hear that you are working on that. Do you think I can help you
with this? I did some groundwork to make it possible. I can help in
coding/reviewing or even can take lead if you want to.

-- 
Ibrar Ahmed


Temporal Table Proposal

2019-02-22 Thread Ibrar Ahmed
Hi,

While working on another PostgreSQL feature, I was thinking that we could
use a temporal table in PostgreSQL. Some existing databases offer this. I
searched for any discussion on the PostgreSQL mailing list, but could not
find any. Maybe my search wasn’t accurate enough:  if anyone can point me
to a discussion, that would be useful.

https://www.percona.com/community-blog/2018/12/14/notes-mariadb-system-versioned-tables/
https://www.mssqltips.com/sqlservertip/3680/introduction-to-sql-server-temporal-tables/

What?
A temporal table feature has two tables “Temporal Table” and “History
Table”. The Temporal Table is where our current tuples are stored. This is
the main table, just like other PostgreSQL tables. The history table is the
other half of the feature and is where all the history of the main table is
stored. This table is created automatically. The history table is used to
query certain data at a certain time, useful for a point in time analysis.
It also offers built-in versioning.

Why?

Normally users write triggers or procedures to write a history of a table’s
data. Some time-sensitive applications will have code to write a data
history somewhere. By having this functionality, PostgreSQL would do it
automatically. For example, if we have a retail table where the price of
each product inventory item is stored. The temporal table would hold the
current price of the product. When we update the price of a product in the
temporal table, then a new row with a timestamp would be added to the
history table. That means on each update of the price, a new row containing
the previous price would be added to the history table. The same would
apply in the case of deletes. When we delete any product from our
inventory, then a row would be added to the history table storing the last
price of the product prior to delete. For any point in time, we can access
the price at which we sold the product.


How?
I was thinking about the implementation of this feature and read the
documentation on the internet. Microsoft SQL Server, for example, offers
such a feature. If we come to the conclusion we should offer the feature, I
will share the complete design.


Here are some ideas I have around this:


 - Syntax.


CREATE TABLE tablename

(

…

start_time DATETIME,

end_time DATETIME,

PERIOD FOR SYSTEM_TIME (start_time, end_time)

)

WITH

(

SYSTEM_VERSIONING = ON (HISTORY_TABLE = tablename_history)

);


The tablename is the temporal table and tablename_history is be the history
table. The name of the history table is optional, in which case, PostgreSQL
will generate a table name. These two columns are a must for a temporal
table “start_time” and “end_time”. The PERIOD FOR SYSTEM_TIME is used to
identify these columns.



ALTER TABLE SET SYSTEM_VERSIONING = ON/OFF


Due to this syntax addition in CREATE/ALTER TABLE, there are some grammar
additions required in the parser.


PERIOD FOR SYSTEM TIME
SYSTEM VERSIONING


 - Catalog Changes.


There are two options, one is to have another catalog pg_temporal which
will contain the information or we could have that information in the
pg_catalog


  Table "public.pg_temporal"

Column  | Type | Collation | Nullable | Default

-+--+---+--+-

temporal_id | oid |  | |

hist_id | oid |  | |

start_date_name | text |   | |

end_date_name   | text |  | |



--
Ibrar Ahmed


Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

2019-02-06 Thread Ibrar Ahmed
On Tue, Jul 3, 2018 at 5:37 PM Moon, Insung 
wrote:

> Dear Tom Lane.
>
> > -Original Message-
> > From: Tom Lane [mailto:t...@sss.pgh.pa.us]
> > Sent: Monday, June 18, 2018 11:52 PM
> > To: Robert Haas
> > Cc: Joe Conway; Masahiko Sawada; Moon, Insung; PostgreSQL-development
> > Subject: Re: [Proposal] Table-level Transparent Data Encryption (TDE)
> and Key Management Service (KMS)
> >
> > Robert Haas  writes:
> > > On Mon, Jun 18, 2018 at 10:12 AM, Joe Conway 
> wrote:
> > >> Not necessarily. Our pages probably have enough predictable bytes to
> > >> aid cryptanalysis, compared to user data in a column which might not
> > >> be very predicable.
> >
> > > Really?  I would guess that the amount of entropy in a page is WAY
> > > higher than in an individual column value.
> >
> > Depending on the specifics of the encryption scheme, having some amount
> of known (or guessable) plaintext may allow breaking
> > the cipher, even if much of the plaintext is not known.  This is
> cryptology 101, really.
> >
> > At the same time, having to have a bunch of independently-decipherable
> short field values is not real secure either, especially
> > if they're known to all be encrypted with the same key.  But what you
> know or can guess about the plaintext in such cases
> > would be target-specific, rather than an attack that could be built once
> and used against any PG database.
>
> Yes. If there is known to guessable data of encrypted data, maybe there is
> a  possibility of decrypting the encrypted data.
>
> But would it be safe to use an additional encryption mode such as GCM or
> XFS to solve this problem?
> (Do not use the same IV)
>
> Thank you and Best regards.
> Moon.
>
>
> >
> >   regards, tom lane
>
>
>
>
>
Hi Moon,

Have you done progress on that patch? I am thinking to work on the project
and found that you are already working on it. The last message is almost
six months old. I want to check with you that are you still working on
that, if yes I can help on that by reviewing the patch etc. If you are not
working on that anymore, can you share your done work (if possible)?
-- 
Ibrar Ahmed


Re: Interesting paper: Contention-Aware Lock Scheduling

2018-12-10 Thread Ibrar Ahmed
On Mon, May 7, 2018 at 10:54 PM Юрий Соколов  wrote:

> 2018-05-04 23:45 GMT+03:00 AJG :
> >
> > Another interesting article from Jan 2018 (Tsinghua University and
> Microsoft
> > Research)
> >
> > http://madsys.cs.tsinghua.edu.cn/publications/TS2018-liu.pdf
> >
> > DudeTx: Durable Transactions Made Decoupled
>
> Cite from pdf:
>
> > The key insight of our solution is decoupling a durable transaction into
> three
> > fully asynchronous steps. (1) Perform: execute the transaction on a
> shadow
> > memory, and produce a redo log for the transaction. (2) Persist: flush
> redo
> > logs of transactions to persistent memory in an atomic manner. (3)
> Reproduce:
> > modify original data in persistent memory according to the persisted
> redo logs.
> > It is essential that we never directly write back dirty data from shadow
> memory
> > to persistent memory – all the updates are realized via the logs.
>
> It is exactly the same Amazon did for its Aurora!
> Using this decoupling, Aurora provides great replication and failover (by
> use of
> replicated and versioned storage both for logs and for data pages).
>
> regards,
> Yura.
>

Hi,

Do we still want to see the patch of that paper (
http://www.vldb.org/pvldb/vol11/p648-tian.pdf)?

-- 
Ibrar Ahmed


Re: pgbench's expression parsing & negative numbers

2018-08-16 Thread Ibrar Ahmed
Patch is good to go from my side.

The new status of this patch is: Ready for Committer


Re: pgbench's expression parsing & negative numbers

2018-08-08 Thread Ibrar Ahmed
The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:not tested

Patch does not apply cleanly on the master branch, anyways I managed that.  
Patch work according to specs, and no issue found. 
The only minor nit is that you can keep the full comments of function strtoint64

/*
 * If not errorOK, an error message is printed out.
 * If errorOK is true, just return "false" for bad input.
 */

Re: Log query parameters for terminated execute

2018-07-26 Thread Ibrar Ahmed
Changed the status as per last email of Tom Lane.

Re: Log query parameters for terminated execute

2018-07-23 Thread Ibrar Ahmed
The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

Review submitted

The new status of this patch is: Waiting on Author


Re: Log query parameters for terminated execute

2018-07-23 Thread Ibrar Ahmed
On Mon, Jul 23, 2018 at 3:05 PM, Sergei Kornilov  wrote:

> Hello
> Thank you for review!
> Well, i can miss some cases. I'm not sure about overall design of this
> patch. Is acceptable add errdetail_params to statement_timeout ereport in
> such way?
>
> After shutdown signal we must be in aborted state, so we mustn't call
> user-defined I/O functions. (quotation from comment to errdetail_params in
> src/backend/tcop/postgres.c ). It seems i can not fix it with current
> design.
>

No its not about calling the function after abort/shutdown. Just start the
server and try to run the program, most of the time you will not get
params.


>
> > ERROR:  canceling statement due to lock timeout at character 13
> Hm, "at character"? How can we get this message? I found only "canceling
> statement due to lock timeout" (without "at character") ereport in
> src/backend/tcop/postgres.c
> Maybe try .. catch in parse state, not in execute?
>

Its really easy to reproduce, just lock the table form another session and
run a "c" program to insert row in the same table.




>
> regards, Sergei
>



-- 
Ibrar Ahmed


Re: Log query parameters for terminated execute

2018-07-23 Thread Ibrar Ahmed
On Sun, Jun 24, 2018 at 1:08 AM, Pavel Stehule 
wrote:

>
>
> 2018-06-23 21:54 GMT+02:00 Sergei Kornilov :
>
>> Hello all
>> We already have feature to logging query parameters. If we use
>> log_statement = 'all' we write parameters before execution and all is fine
>> here. If we use log_min_duration_statement statement is logged obviously
>> after execution, but currently we have no parameters if query was
>> terminated by statement_timeout, lock_timeout or by pg_terminate_backend.
>>
>> I would like have parameters in logs at least for such three cases.
>>
>
> It is good idea - more times I would to see these values
>
> Regards
>
> Pavel
>
>
>> Simple way achieve this is just add errdetail_params to such ereport as
>> in attached patch.
>> Another way is add something like printing global variable
>> debug_query_string in send_message_to_server_log
>> (src/backend/utils/error/elog.c). But i have no good idea how print
>> ParamListInfo correctly. We can not use OidOutputFunctionCall in all cases,
>> right?
>>
>> Another small question is why errdetail_params uses errdetail instead
>> errdetail_log? We assume that the user wants to get their own parameters
>> back (if he set client_min_messages to LOG)?
>>
>> Any feedback is strongly appreciated. Thank you!
>>
>> regards, Sergei
>
>
>
I have reviewed and tested the patch here are my findings about the patch.

Patch applied successfully on master branch
"04269320aed30d3e37c10ae5954eae234d45". There is no compilation issue
with the patch.

statement_timeout: For this I wrote a simple LibPq program to insert into
table. The results are random, some times it logs the param and some time
does not; with the same program. After digging a bit I found that if you
execute just after starting the server it does not log the param and start
logging after subsequent calls. Here is the log

2018-07-23 14:12:13.530 PKT [29165] ERROR:  canceling statement due to
statement timeout

2018-07-23 14:12:13.530 PKT [29165] DETAIL:  parameters: $1 = '16777216',
$2 = 'Foobar'

2018-07-23 14:12:13.530 PKT [29165] STATEMENT:  INSERT INTO tbl_libpq_test
(id, name) VALUES ($1::integer, $2::varchar)


...


2018-07-23 14:13:38.483 PKT [29169] LOG:  shutting down


...


2018-07-23 14:13:38.616 PKT [29901] LOG:  database system is ready to
accept connections

2018-07-23 14:13:39.848 PKT [29910] ERROR:  canceling statement due to
statement timeout

2018-07-23 14:13:39.848 PKT [29910] STATEMENT:  INSERT INTO tbl_libpq_test
(id, name) VALUES ($1::integer, $2::varchar)


lock_timeout: For this I a use the same program to insert into table. I
lock the table in other session and try to execute the program. It does not
log any params at all here is the log.


2018-07-23 14:21:19.165 PKT [30006] ERROR:  canceling statement due to lock
timeout at character 13

2018-07-23 14:21:19.165 PKT [30006] STATEMENT:  INSERT INTO tbl_libpq_test
(id, name) VALUES ($1::integer, $2::varchar)


-- 
Ibrar Ahmed